1116856 - dynamically resize tab mirror video stream based on window size

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: change_size_dynamicly.patch

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: change_size_dynamicly_mobile.patch

With this change, the constraints should actually express the max size of the video stream we want, rather than the exact size we want at the moment.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: change_size_dynamicly_mobile.patch

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8543063 [details] [diff] [review]
change_size_dynamicly.patch

Review of attachment 8543063 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the dummy issue and the malloc failure test apparently missing (I didn't load of the file to see if was checked later, but I suspect not).

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +152,3 @@
>  
> +  mBufWidthMax  = dummy.mMax > cWidth.mMax ? cWidth.mMax : aPrefs.GetWidth(false);
> +  mBufHeightMax  = dummy.mMax > cHeight.mMax ? cHeight.mMax : aPrefs.GetHeight(false);

Isn't dummy just default-initialized here?

@@ +218,5 @@
>      return;
>    }
>  
> +  IntSize size;
> +  if (mBufWidthMax/innerWidth < mBufHeightMax/innerHeight) {

Add comment along the lines of "Maintain source aspect ratio".
How likely is equality here?  Is it worth a test?

@@ +221,5 @@
> +  IntSize size;
> +  if (mBufWidthMax/innerWidth < mBufHeightMax/innerHeight) {
> +    size = IntSize(mBufWidthMax, mBufWidthMax * ((float)innerHeight/innerWidth));
> +  } else {
> +    size = IntSize(mBufHeightMax * ((float)innerWidth/innerHeight), mBufHeightMax);

I'd prefer for clarity ... (((float) innerWidth)/innerHeight),

@@ +227,3 @@
>  
> +  if (mDataSize < (size_t)(size.width * size.height * 4)) {
> +    mDataSize = size.width * size.height * 4;

I'd prefer that the constants "4" be replaced with sizeof(something) or a reference to the actual datatype for the storage used.

@@ +227,4 @@
>  
> +  if (mDataSize < (size_t)(size.width * size.height * 4)) {
> +    mDataSize = size.width * size.height * 4;
> +    mData = (unsigned char*)malloc(mDataSize);

nit: space after cast.  Or better: static_cast<>.

The (fallible) allocation via malloc() MUST be checked.

@@ +262,5 @@
>      return;
>    }
>    nsRefPtr<gfxContext> context = new gfxContext(dt);
> +  context->SetMatrix(context->CurrentMatrix().Scale((float)size.width/innerWidth,
> +                                                    (float)size.height/innerHeight));

Same comment about ()'s on the casts (I realize it was that way already).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8543073 [details] [diff] [review]
change_size_dynamicly_mobile.patch


>diff --git a/toolkit/modules/secondscreen/RokuApp.jsm b/toolkit/modules/secondscreen/RokuApp.jsm

>-        { aspectRatio: cWidth / cHeight }
>+        { aspectRatio: MAX_WIDTH/MAX_HEIGHT }

space around the '/'

LGTM assuming the platform side is functional. I also want Randall to take a look.

Comment 5

[Randall Barker [:rbarker]]

Looks okay to me.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 8543063 [details] [diff] [review]
> change_size_dynamicly.patch
> 
> Review of attachment 8543063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the dummy issue and the malloc failure test apparently missing (I
> didn't load of the file to see if was checked later, but I suspect not).
> 
> ::: dom/media/webrtc/MediaEngineTabVideoSource.cpp
> @@ +152,3 @@
> >  
> > +  mBufWidthMax  = dummy.mMax > cWidth.mMax ? cWidth.mMax : aPrefs.GetWidth(false);
> > +  mBufHeightMax  = dummy.mMax > cHeight.mMax ? cHeight.mMax : aPrefs.GetHeight(false);
> 
> Isn't dummy just default-initialized here?
I'm using this to get the default for max, changed the name to defaultRange for clarity.

 
> @@ +218,5 @@
> >      return;
> >    }
> >  
> > +  IntSize size;
> > +  if (mBufWidthMax/innerWidth < mBufHeightMax/innerHeight) {
> 
> Add comment along the lines of "Maintain source aspect ratio".
> How likely is equality here?  Is it worth a test?
probably somewhat likely, but you'll wind up with the same numbers modulo float math errors.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: change_size_dynamicly.patch

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8548574 [details] [diff] [review]
change_size_dynamicly.patch

Review of attachment 8548574 [details] [diff] [review]:
-----------------------------------------------------------------

r- on the stride issue (sorry I didn't pick up on it before), and it pre-existed this patch, but let's fix it since it's easy and we're redoing this section.

Also: file a followup bug to redo NotifyPull/AppendToTrack to work like MediaEngineWebrtcVideo does now (push frames in with duration 1, then let NotifyPull extend that).  Improved framerate (esp on B2G or if audio callbacks are slower/for-more-data) and reduces delay.

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +229,4 @@
>  
> +  if (mDataSize < static_cast<size_t>(size.width * size.height * 4)) {
> +    mDataSize = size.width * size.height * 4;
> +    mData = static_cast<unsigned char*>(malloc(mDataSize));

Hmmm.  First: is there a way to ask (or a define for) the size of gfxImageFormat::RGB24?  If not, ok.

Second: below we get the stride for size.... but we're not using it for allocating the buffer it applies to.  If stride != width, the (correct me if I'm wrong) this allocation could be too small.  See gfx/layers/basic/BasicImages.cpp:

  mStride = gfxASurface::FormatStrideForWidth(iFormat, size.width);
  mDecodedBuffer = AllocateBuffer(size.height * mStride);

@@ +233,4 @@
>    }
>  
> +  if (!mData)
> +    return;

Missing {}

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Randell Jesup [:jesup] from comment #8)
> Comment on attachment 8548574 [details] [diff] [review]
> change_size_dynamicly.patch
> 
> Review of attachment 8548574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webrtc/MediaEngineTabVideoSource.cpp
> @@ +229,4 @@
> >  
> > +  if (mDataSize < static_cast<size_t>(size.width * size.height * 4)) {
> > +    mDataSize = size.width * size.height * 4;
> > +    mData = static_cast<unsigned char*>(malloc(mDataSize));
> 
> Hmmm.  First: is there a way to ask (or a define for) the size of
> gfxImageFormat::RGB24?  If not, ok.
yes, gfxASurface::BytePerPixelFromFormat()

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: change_size_dynamicly.patch

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Randell Jesup [:jesup] from comment #8)
> Comment on attachment 8548574 [details] [diff] [review]
> change_size_dynamicly.patch
> 
> Review of attachment 8548574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- on the stride issue (sorry I didn't pick up on it before), and it
> pre-existed this patch, but let's fix it since it's easy and we're redoing
> this section.
> 
> Also: file a followup bug to redo NotifyPull/AppendToTrack to work like
> MediaEngineWebrtcVideo does now (push frames in with duration 1, then let
> NotifyPull extend that).  Improved framerate (esp on B2G or if audio
> callbacks are slower/for-more-data) and reduces delay.

Can you elaborate? The only difference I see between MediaEngineTabVideoSource::NotifyPull()[1] and  MediaEngineWebRTCVideoSource::NotifyPull()[2] which calls MediaEngineCameraVideoSource::AppendToTrack()[3] is that MediaEngineTabVideoSource is dealing with CairoImages and therefore downcasts to a layers::Image and that it gets the size directly from the image rather than mHeight and mWidth.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp#194
[2] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineWebRTCVideo.cpp#124
[3] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#52

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)
> (In reply to Randell Jesup [:jesup] from comment #8)

> > Also: file a followup bug to redo NotifyPull/AppendToTrack to work like
> > MediaEngineWebrtcVideo does now (push frames in with duration 1, then let
> > NotifyPull extend that).  Improved framerate (esp on B2G or if audio
> > callbacks are slower/for-more-data) and reduces delay.
> 
> Can you elaborate? The only difference I see between
> MediaEngineTabVideoSource::NotifyPull()[1] and 
> MediaEngineWebRTCVideoSource::NotifyPull()[2] which calls
> MediaEngineCameraVideoSource::AppendToTrack()[3] is that
> MediaEngineTabVideoSource is dealing with CairoImages and therefore
> downcasts to a layers::Image and that it gets the size directly from the
> image rather than mHeight and mWidth.

You need to look at DeliverFrame() in VideoSource - we push it in via AppendToTrack() (with a delta time of 1 tick) and then let NotifyPull extend the time as needed.  This gets the frame in as soon as possible.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c74a032abad8
https://hg.mozilla.org/mozilla-central/rev/6be66b737116
https://hg.mozilla.org/mozilla-central/rev/df6b421df37f

Comment 14

[Randall Barker [:rbarker]]

This patch breaks tab mirroring on Roku and desktop Roku simulator.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

I note that size is being adjusted for aspect ratio now before determining the stride, whereas before stride was being determined solely from mBufWidthMax/etc.  stride is used for two things; allocating the buffer (size), and for CreateDrawTargetForData.  I imagine this is where the issue is.  Stride seems to be off in the image on bug 1124512 by ~2 pixels/line luma, 1 per line chroma

Comment 16

[Randall Barker [:rbarker]]

After further investigating, it appears Bug 1116859 introduced the original problem seen in Bug 1124512

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

I think the real problem is that our RGB to YUV conversion can't handle strides that aren't divisible by 4. We worked around that in the js code before (passing constraints that were divisible by 4). With the patches on this bug, the constraints are treated as mins and maxes and now we need to handle that divisible by 4 restriction in the video source code.