Closed Bug 1116856 Opened 5 years ago Closed 5 years ago

dynamically resize tab mirror video stream based on window size

Categories

(Core :: WebRTC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
fennec 36+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch change_size_dynamicly.patch (obsolete) — Splinter Review
No description provided.
Attachment #8543063 - Flags: review?(rjesup)
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.
Assignee: nobody → blassey.bugs
Attachment #8543065 - Flags: review?(mark.finkle)
Attachment #8543065 - Attachment is obsolete: true
Attachment #8543065 - Flags: review?(mark.finkle)
Attachment #8543073 - Flags: review?(mark.finkle)
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).
Attachment #8543063 - Flags: review?(rjesup) → review-
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.
Attachment #8543073 - Flags: review?(mark.finkle)
Attachment #8543073 - Flags: review+
Attachment #8543073 - Flags: feedback?(rbarker)
Looks okay to me.
Attachment #8543073 - Flags: feedback?(rbarker)
tracking-fennec: ? → 36+
(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.
Attached patch change_size_dynamicly.patch (obsolete) — Splinter Review
Attachment #8543063 - Attachment is obsolete: true
Attachment #8548574 - Flags: review?(rjesup)
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 {}
Attachment #8548574 - Flags: review?(rjesup) → review-
(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()
Attachment #8548574 - Attachment is obsolete: true
Attachment #8551957 - Flags: review?(rjesup)
(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
Flags: needinfo?(rjesup)
(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.
Flags: needinfo?(rjesup)
Attachment #8551957 - Flags: review?(rjesup) → review+
This patch breaks tab mirroring on Roku and desktop Roku simulator.
Depends on: 1124512
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
Flags: needinfo?(blassey.bugs)
After further investigating, it appears Bug 1116859 introduced the original problem seen in Bug 1124512
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.
Flags: needinfo?(blassey.bugs)
You need to log in before you can comment on or make changes to this bug.