Re: maxHeight and maxWidth

On Fri, Feb 12, 2016 at 1:05 PM, Adam Roach <abr@mozilla.com> wrote:

> On 2/12/16 14:36, Peter Thatcher wrote:
>
> We've discussed the downsides of maxHeight/maxWidth in many f2f meetings,
> so we can just go dig up the notes from before.
>
>
> The only arguments I recall being raised is "we don't need them, so why
> specify them?" But now we have implementors coming to us saying they *do*
> need them, so that rationale kind of crumbles.
>

​I recall the discussions being much longer and deeper than just that.​



>
>
> But I recall the case against maxHeight and maxWidth is the complexity and
> ambiguity that it introduces, as well as the duplication of control.  You
> can already control the height and width of the track via constraints on
> the track.
>
>
> I don't think that argues one way or another: pretty much everything we
> can do on the senders/tranceivers can be done on the individual streams.
> The whole point of sender/tranceiver controls is to modify what goes on the
> wire as compared to the properties of the stream.
>

That just means that we can add duplicate controls.  That doesn't mean we
should add duplicate controls.  ​
​


> In terms of complexity, I'd love to hear you expand on your concerns here.
> Perhaps I'm being naïve, but once you implement the ability to handle
> scaleResolutionDownBy, you simply need to add excruciatingly simple logic
> on a per-frame basis along the lines of:
>
> if (stream.current_resolution != stream.previous_resolution) {
>   resizeScale =  scaleResolutionDownBy;
>   if (stream.width / resizeScale > maxWidth) {
>     resizeScale = stream.width / maxWidth;
>   }
>   if (stream.height / resizeScale > maxHeight) {
>     resizeScale = stream.height / maxHeight;
>   }
> }
>

​You didn't take into account rotation.
​
​But seeing that code does give me an idea for something even better; add
an event to a video track: onresolutionchanged.  Then, whenever the video
track's resolution changes, the Javascript can reset the scale:

track.onresolutionchanged = function(evt) {
  var params = sender.getParameters();
  params.encodings[1].scaleResolutionDownBy = evt.height / 90;
  sender.setParameters(params);
}

​It has the downside that a frame or two of the old scale might escape.
But assuming the remote endpoint drops frames of incompatible size (or the
SFU does) rather than explode, it shouldn't be a big problem.​

​That would be a generally useful event to fire anyway, so I'd be more in
favor of adding it than adding .maxHeight and .maxWidth to the
EncodingParameters.​  And it would leave us with orthogonal controls, not
duplicate ones.

Plus, it would allow the JS to handle an edge case which you have not
brought up:  what happens when the video resolution of the screencast drops
below 90 pixels high (a very small window)?  Then your remote endpoint will
get something below 90 and explode, because there is no minHeight.



>   And if we did add a maxHeight and the frame is too big, it's not clear
> what should happen: scaling, cropping, pillar boxes, etc.
>
>
> Oh, that's easy. Unless we want to make the interactions unnecessarily
> complex, it needs to be the same thing as we get for scaleResolutionDownBy:
> scaling.
>

In that case, we should call them scaleResolutionDownToHeight and
scaleResolutionDownToWidth.

But I still don't see your "remote simulcast endpoint can't scale down" use
case worth adding these, especially when "poor man's simulcast" works fine
for that use case, and track.onresolutionchanged could as well, and be more
useful.
​



>
>
>
> --
> Adam Roach
> Principal Platform Engineer
> Office of the CTO
>