1169125 - Occasional green frames in remote video when streaming a CanvasCaptureMediaStream over WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Andreas Pehrson [:pehrsons]]

STR:
1. Go to https://mozilla.github.io/webrtc-landing/canvas_swap.html
2. Click start

Expected: smooth video on the remote side

Actual: mostly smooth video, but with occasional green frames inserted on the remote side

Comment 1

[Andreas Pehrson [:pehrsons]]

This is because of DataSourceSurface::GetData() returns null when we try to read the video frame in MediaPipeline.

Not sure why we can't read the surface, but I'll patch it up to use the new API Map/Unmap instead of the deprecated GetData/GetStride, and guard for errors.

It ended up green (I've seen it pink too) because the conversion from ARGB to I420 would exit early and we'd then send just-initialized 0's over the wire.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

Bug 1169125 - Check for Map() failure before sending out video frame in MediaPipeline. r=bwc

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8615806 [details]
MozReview Request: Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

https://reviewboard.mozilla.org/r/10367/#review9115

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1211
(Diff revision 1)
> -        libyuv::ARGBToI420(static_cast<uint8*>(surf->GetData()), surf->Stride(),
> +        libyuv::ARGBToI420(static_cast<uint8*>(map.mData), map.mStride,

Maybe what we really want to be doing is bailing on any error here. That will catch cases where surf->GetData() is null (this particular bug, if I understand correctly), and other error conditions (although the other error conditions don't seem to be possible).

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1218
(Diff revision 1)
> -        libyuv::RGB565ToI420(static_cast<uint8*>(surf->GetData()), surf->Stride(),
> +        libyuv::RGB565ToI420(static_cast<uint8*>(map.mData), map.mStride,

We should also be checking here.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1226
(Diff revision 1)
>          MOZ_ASSERT(PR_FALSE);

We should definitely be bailing here in release, otherwise we send a video frame of uninitialized memory.

Comment 4

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8615806 [details]
MozReview Request: Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

Bug 1169125 - Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8615806 [details]
MozReview Request: Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

Bug 1169125 - Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

Comment 6

[Andreas Pehrson [:pehrsons]]

I addressed your comments Byron. I also, as a general cleanup thing, removed the restriction on surfaces being YCBCR or CAIRO. YCBCR (still special case since it doesn't need converting) and any surfaces where we can read out the bits should be good. This also happens to fix issue 1 of bug 1171907.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8615806 [details]
MozReview Request: Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

https://reviewboard.mozilla.org/r/10367/#review9211

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1234
(Diff revision 2)
> +        return;
> +    }
> +    if (rv != 0) {
> +      MOZ_MTLOG(ML_ERROR, "RGB to I420 conversion failed");
> +      return;
>      }
> +    data->Unmap();

We want to Unmap before bailing, right?

Comment 8

[Andreas Pehrson [:pehrsons]]

(In reply to Byron Campen [:bwc] from comment #7)
> Comment on attachment 8615806 [details]
> MozReview Request: Bug 1169125 - Allow sending any DataSourceSurface-backed
> image over WebRTC and fix failure cases. r=bwc
> 
> https://reviewboard.mozilla.org/r/10367/#review9211
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1234
> (Diff revision 2)
> > +        return;
> > +    }
> > +    if (rv != 0) {
> > +      MOZ_MTLOG(ML_ERROR, "RGB to I420 conversion failed");
> > +      return;
> >      }
> > +    data->Unmap();
> 
> We want to Unmap before bailing, right?

Oh, we do indeed. Good catch!

I just got r+ on bug 1162357. Let me land that first and we can use the scope for unmapping.

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8615806 [details]
MozReview Request: Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1169125 - Part 2. Use UniquePtr for scoped delete of yuv data in MediaPipeline. r=bwc

Bug 1169125 - Part 2. Use UniquePtr for scoped delete of yuv data in MediaPipeline. r=bwc

Comment 11

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10)
> Created attachment 8617111 [details]
> MozReview Request: Bug 1169125 - Part 2. Use UniquePtr for scoped delete of
> yuv data in MediaPipeline. r=bwc
> 
> Bug 1169125 - Part 2. Use UniquePtr for scoped delete of yuv data in
> MediaPipeline. r=bwc

We also need to make sure |yuv| doesn't leak, which this patch fixes.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8615806 [details]
MozReview Request: Bug 1169125 - Part 1. Allow sending any DataSourceSurface-backed image over WebRTC and fix failure cases. r=bwc

https://reviewboard.mozilla.org/r/10367/#review9321

Ship It!

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8617111 [details]
MozReview Request: Bug 1169125 - Part 2. Use UniquePtr for scoped delete of yuv data in MediaPipeline. r=bwc

https://reviewboard.mozilla.org/r/10545/#review9323

Ship It!

Comment 14

[Andreas Pehrson [:pehrsons]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c41769697be

Comment 15

[Andreas Pehrson [:pehrsons]]

Please land on top of bug 1162357.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92b2503d2ee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3af3b1cf582

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/92b2503d2ee7
https://hg.mozilla.org/mozilla-central/rev/c3af3b1cf582