1081409 - mozCaptureStream video over peerConnection almost works (wrong length)

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: workaround patch (NOT FOR LANDING)

With the placeholder-track patch from Bug 1070127 and the attached workaround-patch, I'm able to run the URL test script (coming next) which sends a mozCaptureStream'ed video-file over peerConnection.

Without the workaround-patch it doesn't play and triggers an assert in MediaPipeline.cpp instead:

  MOZ_ASSERT(length == I420SIZE(width, height));

Removing the assert isn't enough; The frame length has to be correct (852*480 PLANAR_YCBCR - should be 613440, was 613480) See workaround-patch for details.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: testfile.webm

Test script doesn't work cross-origin so I'm uploading test file here.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: pc_test_capture.html - test case

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Filed Bug 1081819 for the audio part.

Comment 4

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

Attachment: Fix forwarding of captured MediaStreams to PeerConnections

relax the tests on length since TextureClients may add other fields/structures to the buffer the YUV data is in

Comment 5

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

we'll also want to write a mochitest for forwarding video

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8504796 [details] [diff] [review]
Fix forwarding of captured MediaStreams to PeerConnections

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

r=me with comment fix.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +1219,1 @@
>      // XXX Consider making this a non-debug-only check if we ever implement

Patch makes this comment obsolete.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: test_peerConnection_capturedVideo.html - works in browser

The oddest thing. Works in browser but not in mochitest harness. Timing?

Test is doctored to work in browser and use already uploaded file, though I've tested that the files don't make a difference.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8505245 [details]
test_peerConnection_capturedVideo.html - works in browser

Removing since it had .js dependencies that made it not run in bugzilla. Anyways, if you put this file ontop of the next patch, copy the dir to a web server and run it in a browser it works (whereas the mochitest does not for some reason).

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: test mozCaptureStream over peerConnection WIP

Here's the mochitest I added but can't get to succeed in the mochitest harness, even though the same test running in a browser seems to. Ideas as to why are welcome.

Comment 10

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

Make sure it's adding the tracks to the right peerconnection....  

Also, I notice there's a "Skipping GUM" line; that means attachMedia() isn't called (which likely is fine and correct, but I just want to make sure skipping it didn't leave any landmines or unfinished items).

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The "Skipping GUM" is intentional. I didn't need to remove that step as I found it's a no-op when I omit attachMedia. I'm addTracking manually to pcLocal._pc.

Regardless, that this test works in the browser (comment 8) I hope rules out most pilot errors.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a52b2b37e42a
We can land the tests on a follow-up bug

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e655fbbc6e9f

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3028861&repo=mozilla-inbound

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8504796 [details] [diff] [review]
Fix forwarding of captured MediaStreams to PeerConnections

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +1197,4 @@
>      uint8_t *y = data->mYChannel;
>  #ifdef DEBUG
>      uint8_t *cb = data->mCbChannel;
>      uint8_t *cr = data->mCrChannel;

Ugh should have caught that on review. cb and cr need to be defined all the time now, not just in debug.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Randell Jesup [:jesup] from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a52b2b37e42a
> We can land the tests on a follow-up bug

I agree. The problems we're with the tests are surely unrelated to this patch.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfafff4ff07

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bfafff4ff07