868405 - Implement MediaStreamTrack::enabled

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: add some MOZ_OVERRIDEs

Not really part of this bug, but I did it...

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This works. However, perhaps because it's late, I don't understand why disabling the video track gives solid green output. I'm creating a PLANAR_YCBCR surface whose data values are all zero, and I thought zero luminance would give black.

It turned out to be easiest to pass the disabled-ness state of a video frame in VideoFrame rather than try to create a black image in ApplyTrackDisabling. In ApplyTrackDisabling we don't have access to an ImageContainer we can use to create a new image.

This needs tests still.

Comment 3

[Timothy B. Terriberry (:derf)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> This works. However, perhaps because it's late, I don't understand why
> disabling the video track gives solid green output. I'm creating a
> PLANAR_YCBCR surface whose data values are all zero, and I thought zero
> luminance would give black.

Because all-zero Y'CbCr data is out-of-gamut for RGB. Green winds up as a large positive value, and red and blue wind up as large negative values, which then get clamped to 0, leaving only the green. You want Y'=0, Cb,Cr=128.

Comment 4

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

Comment on attachment 746810 [details] [diff] [review]
fix

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

r-, f? to derf for his comments

::: content/media/MediaStreamGraph.cpp
@@ +808,5 @@
> +{
> +  uint8_t zero[1] = { 0 };
> +
> +  PlanarYCbCrImage::Data data;
> +  data.mYChannel = data.mCbChannel = data.mCrChannel = zero;

See comments on MediaPipeline; this isn't black, and 1x1 may be the wrong size.

@@ +860,5 @@
> +      nsRefPtr<Image> image =
> +        output->GetImageContainer()->CreateImage(formats, 1);
> +      if (image) {
> +        // Sets the image to a single black pixel, which will be scaled to fill
> +        // the rendered size.

Users of getUserMedia() may not be set up to do scaling.  The best choice is a black frame the same size as the frame of real data.  And yes, this may mean allocating an image to do this.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +785,5 @@
>      TrackRate rate,
>      VideoChunk& chunk) {
> +  if (chunk.mFrame.GetForceBlack()) {
> +    // Send a single 2x2 black image.
> +    uint8_t zeroes[6] = { 0, 0, 0, 0, 0, 0 };

For various reasons, I think a 16x16 black image is far preferable (video codecs typically work on 16x16 macroblocks, and even if they in theory support smaller sizes I'd be concerned as to how much testing they'd get).  Also, a (very) minor point but changing resolutions in some codecs will cause extra packets to be sent.

Another issue to consider is that size changes are allowed by most codecs, but applications (especially non-codec applications that asked for a mandatory capture size from getUserMedia()) may not expect enable/disable to change the size.  As this is in Pipeline that's not relevant.

So the obvious choices are 16x16 (minimum "safe" size), and same-as-last-frame (with some default if there was no last frame).

Also: in YCrCb, black is not 0,0,0.  It's 0x10, 0x80, 0x80 for BT.601 (0x00, 0x80, 0x80 would be "blacker-than-black" in analog video terms (and some digital->analog converters will do that, others will clamp IIRC), and some camera chips for example use "out-of-bounds" Y and maybe cr/cb values for sync/etc info).  The value for pure white is 235, 0x80, 0x80 (though in theory for white and black the chroma values are irrelevant).  Y has a range of 16-235.  Cr and Cb have a range of 16-239.

If we provide a black image in MediaStreamGraph (see earlier comments), we shouldn't need or want to do anything special here.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randell Jesup [:jesup] from comment #4)
> Users of getUserMedia() may not be set up to do scaling.  The best choice is
> a black frame the same size as the frame of real data.  And yes, this may
> mean allocating an image to do this.

I can do that, but it'll have to happen in MediaPipeline because of the second paragraph of comment #2.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

This produces black images. Also, we feed the WebRTC video encoder with a black image the same size as the image would be if the track was enabled.

I left the media element output image as a 1x1 black pixel because that should be fine for our media element rendering code.

Comment 7

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

Comment on attachment 747760 [details] [diff] [review]
fix v2

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

r+ with removal of size rounding

::: content/media/MediaStreamGraph.cpp
@@ +812,5 @@
> +  data.mYChannel = blackPixel;
> +  data.mCbChannel = blackPixel + 1;
> +  data.mCrChannel = blackPixel + 2;
> +  data.mYStride = data.mCbCrStride = 1;
> +  data.mPicSize = data.mYSize = data.mCbCrSize = gfxIntSize(1, 1);

I'm still a little leery of this since our <video> elements aren't the only sink I can imagine (and even if they are, applications may be surprised by <video> size changes from a camera - though again, they probably shouldn't be in theory, but I'm betting in practice they'd be surprised).  This can be discussed in followups if needed, however.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +799,5 @@
> +    gfxIntSize size = img ? img->GetSize() : chunk.mFrame.GetIntrinsicSize();
> +    // Increase dimensions to the nearest multiple of 16
> +    size.width = (size.width + 15) & ~15;
> +    size.height = (size.height + 15) & ~15;
> +    uint32_t length = (size.width*size.height*3)/2;

If we're already being supplied with a given size, there's no _additional_ need to round it up when providing black pixels (if so, we'd also need to round it up for all images, and fill in or center in some way).  Note that sources *should* provide %16=0 sizes normally.  So we should be able to remove this part.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randell Jesup [:jesup] from comment #7)
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +799,5 @@
> > +    gfxIntSize size = img ? img->GetSize() : chunk.mFrame.GetIntrinsicSize();
> > +    // Increase dimensions to the nearest multiple of 16
> > +    size.width = (size.width + 15) & ~15;
> > +    size.height = (size.height + 15) & ~15;
> > +    uint32_t length = (size.width*size.height*3)/2;
> 
> If we're already being supplied with a given size, there's no _additional_
> need to round it up when providing black pixels (if so, we'd also need to
> round it up for all images, and fill in or center in some way).  Note that
> sources *should* provide %16=0 sizes normally.  So we should be able to
> remove this part.

So if we feed a video resource with odd width or height via mozCaptureStream to a PeerConnection, we currently just crash?

We need to decide what constraints, if any, we're going to have on video frames in the MSG, and there's no time like the present! "None at all" is my preferred option.

Comment 9

[Timothy B. Terriberry (:derf)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> So if we feed a video resource with odd width or height via mozCaptureStream
> to a PeerConnection, we currently just crash?

We certainly would if fed an odd-sized stream from gUM (though we control the resolution we ask for, so this should never happen). I don't think we've tested the mozCaptureStream path.

> We need to decide what constraints, if any, we're going to have on video
> frames in the MSG, and there's no time like the present! "None at all" is my
> preferred option.

I agree. The thumbnails used by Hangouts currently have odd dimensions.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I filed bug 871416 on supporting any size of video frames in MediaPipelineTransmit::PipelineListener::ProcessVideoChunk.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randell Jesup [:jesup] from comment #7)
> I'm still a little leery of this since our <video> elements aren't the only
> sink I can imagine (and even if they are, applications may be surprised by
> <video> size changes from a camera - though again, they probably shouldn't
> be in theory, but I'm betting in practice they'd be surprised).  This can be
> discussed in followups if needed, however.

Applications should not be able to observe this 1x1 pixel size, because normally when rendering video the image is scaled to the video's intrinsic size before being scaled to fit the video element's content-box. So we should be fine.

I have just realized that canvas.drawImage(video) will render using the image size, without scaling to the intrinsic size, which is broken for videos with non-1:1 pixel aspect ratios. I'll fix that.

> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +799,5 @@
> > +    gfxIntSize size = img ? img->GetSize() : chunk.mFrame.GetIntrinsicSize();
> > +    // Increase dimensions to the nearest multiple of 16
> > +    size.width = (size.width + 15) & ~15;
> > +    size.height = (size.height + 15) & ~15;
> > +    uint32_t length = (size.width*size.height*3)/2;
> 
> If we're already being supplied with a given size, there's no _additional_
> need to round it up when providing black pixels (if so, we'd also need to
> round it up for all images, and fill in or center in some way).  Note that
> sources *should* provide %16=0 sizes normally.  So we should be able to
> remove this part.

Tell you what, I'll just take these adjustments out and MOZ_ASSERT that the width and height are even so YUV420 at least makes sense. I still think 871416 needs to be fixed.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

How about this?

Comment 13

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

Comment on attachment 748697 [details] [diff] [review]
fix v3

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

I'll address the odd issue in the other bug - not so easy

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e274bbbd493b
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e88ec6ac1f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e274bbbd493b
https://hg.mozilla.org/mozilla-central/rev/45e88ec6ac1f
https://hg.mozilla.org/mozilla-central/rev/72672b33604b

Comment 16

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

Comment on attachment 748697 [details] [diff] [review]
fix v3

[Approval Request Comment]
Note: this is for this patch, the other patch here, and the bustage fix that landed with it (see checkin report)

Bug caused by (feature/regressing bug #): N/A

User impact if declined: Users won't be able to mute audio/video tracks.  This is a serious annoyance in a call or especially conference.  We'd like to avoid having this missing for more than one user release (FF22 won't have this).

Testing completed (on m-c, etc.): on m-c

Risk to taking this patch (and alternatives if risky): low risk.  Nothing tricky or all that unusual.

String or IDL/UUID changes made by this patch: none

Comment 17

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

Comment on attachment 746808 [details] [diff] [review]
add some MOZ_OVERRIDEs

[Approval Request Comment]
see other patch

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9ffcebc6abc
https://hg.mozilla.org/releases/mozilla-aurora/rev/487b28716f46
https://hg.mozilla.org/releases/mozilla-aurora/rev/71a5638a8fdf

Comment 19

[Jason Smith [:jsmith]]

Verified through doing a lot of getUserMedia and basic peer connection calling with changing enabled to true and false to verify the correct track was being disabled (e.g. black video on video track enabled being set to false).

Comment 20

[Eric Rescorla (:ekr)]

Attachment: Clean up MediaPipeline code from

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 763559 [details] [diff] [review]
Clean up MediaPipeline code from

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

The original patch contained:

1. An unused function
2. Weird ordering of a null check for img.

I've cleaned these up.

Comment 22

[Eric Rescorla (:ekr)]

Re-opened for cleanup.

Comment 23

[Jason Smith [:jsmith]]

(In reply to Eric Rescorla (:ekr) from comment #22)
> Re-opened for cleanup.

Please file followups for updates to existing patches and do not reopen bugs for this purpose. This screws up our tree management landing process, our release tracking process, and our QA verification process when we reopen bugs, as data confusion happens over what needs to be tracked for a release. As such, this followup patch is cleanup and should be tracked in a separate bug, so I'm reclosing the bug.

Comment 24

[Jeremie Patonnier :Jeremie]

Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885239