936549 - Tab sharing capture device won't stream

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

the tab sharing capture device added in bug 742832 works fine for displaying the video locally, but breaks when trying to stream that video over a peer connection, which was basically the point of doing it in the first place.

Comment 1

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

Attachment: rgb_to_yuv_pipeline.patch

Our pipeline just punts on non-planar YUV images. I don't know if the encoders will work with RGB data or not (I tried making it work for about a day), but doing the RGB to YUV conversions at least gets us moving.

Comment 2

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

Attachment: use_FormatStrideForWidth.patch

while testing the various formats, noticed that we have a magic number there for bpp. This patch uses the utility function to get the stride. No functional change.

Comment 3

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

Comment on attachment 829411 [details] [diff] [review]
rgb_to_yuv_pipeline.patch

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

r- for the malloc issue.  Check and fail I think, don't convert to infallible.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +932,5 @@
> +    gfxIntSize size = rgb->GetSize();
> +    int half_width = (size.width + 1) >> 1;
> +    int half_height = (size.height + 1) >> 1;
> +    int c_size = half_width * half_height;
> +    int buffer_size = size.width * size.height + c_size * 2;

perhaps:
c_size = gfxIntSize(half_width, half_height);
buffer_size = gfxIntSize(size.width, size.height) + 2 * c_size;

Just pedanticism.

@@ +933,5 @@
> +    int half_width = (size.width + 1) >> 1;
> +    int half_height = (size.height + 1) >> 1;
> +    int c_size = half_width * half_height;
> +    int buffer_size = size.width * size.height + c_size * 2;
> +    uint8* yuv = (uint8*) malloc(buffer_size);

malloc() is fallible still last I knew

@@ +939,5 @@
> +
> +    switch (surf->Format()) {
> +      case gfxImageFormatARGB32:
> +      case gfxImageFormatRGB24:
> +        libyuv::ARGBToI420(static_cast<uint8*>(surf->Data()), surf->Stride(), yuv, size.width, yuv + size.width * size.height, half_width, yuv + size.width * size.height + c_size, half_width, size.width, size.height);

line breaks

@@ +942,5 @@
> +      case gfxImageFormatRGB24:
> +        libyuv::ARGBToI420(static_cast<uint8*>(surf->Data()), surf->Stride(), yuv, size.width, yuv + size.width * size.height, half_width, yuv + size.width * size.height + c_size, half_width, size.width, size.height);
> +        break;
> +      case gfxImageFormatRGB16_565:
> +        libyuv::RGB565ToI420(static_cast<uint8*>(surf->Data()), surf->Stride(), yuv, size.width, yuv + size.width * size.height, half_width, yuv + size.width * size.height + c_size, half_width, size.width, size.height);

Ditto

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 829411 [details] [diff] [review]
rgb_to_yuv_pipeline.patch

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

Drive-by-review: I think currently 

conduit->SendVideoFrame(yuv, buffer_size, size.width, size.height, mozilla::kVideoI420, 0);

already ends up calling libyuv somewhere in the path before the VP8 encoder, in order to transfer the frame to whatever it needs. So you probably want to put mozilla::kWhateverIHaveHere in there instead of kVideoI420 and let libyuv do the transformation in one go. You'll avoid having the transformation spread across two code layers and it'll perform better as well.

You pointed me to:
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#802
on IRC, but obviously that doesn't mean you can't add new things. This is just calling into:
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc#228

Comment 5

[Gian-Carlo Pascutto [:gcp]]

...and bugzilla ate my last sentence. The last function mentioned calls libyuv here:
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc#286

Comment 6

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

Right - if we've configured the channel to send at a resolution, it will scale frames to be sent to that size (though note that we have code to reconfigure the VideoConduit to match the getUserMedia frame size, so be careful that's not getting triggered - either avoid it for this case, or pre-scale).

Comment 7

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

Attachment: rgb_to_yuv_pipeline.patch

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 829411 [details] [diff] [review]
> rgb_to_yuv_pipeline.patch
> 
> Review of attachment 829411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the malloc issue.  Check and fail I think, don't convert to
> infallible.
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +932,5 @@
> > +    gfxIntSize size = rgb->GetSize();
> > +    int half_width = (size.width + 1) >> 1;
> > +    int half_height = (size.height + 1) >> 1;
> > +    int c_size = half_width * half_height;
> > +    int buffer_size = size.width * size.height + c_size * 2;
> 
> perhaps:
> c_size = gfxIntSize(half_width, half_height);
> buffer_size = gfxIntSize(size.width, size.height) + 2 * c_size;
I don't think that works because gfxIntSize doesn't cast to an int.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/101767d0f96e
https://hg.mozilla.org/mozilla-central/rev/3fdb887ba7d7

Comment 9

[Ioana (away)]

Any demo page QA can test this with? Should this also work on desktop or only on mobile devices?

Comment 10

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

we need the UI to expose this feature (bug 928096) in order to test it. I'd suggest waiting until that lands.

Comment 11

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

Although the desktop UI bug (bug 923228) has an addon you could try to use to test this.

Comment 12

[u279076]

Flagging for verification dependent upon bug 928096.