1054704 - Fake H.264 encoder doesn't work in mochitests

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

STR:
1. Make the video elements visible with the attached patch.
2. Run the H.264 and VP8 tests, as-in:
./mach mochitest-plain dom/media/tests/mochitest/test_peerConnection_basicH264Video.html
 ./mach mochitest-plain dom/media/tests/mochitest/test_peerConnection_basicVideo.html

Expected behavior:
Both display four video panes with video.

Actual behavior:
The VP8 test behaves as expected but the H.264 test only displays local
video.

The stats are also consistent with failure. Here are the VP8 stats:
808 INFO PeerConnectionWrapper (pcLocal): Got stats: {"outbound_rtcp_video_1":{"id":"outbound_rtcp_video_1","timestamp":1408234289060.3262,"type":"inboundrtp","isRemote":true,"remoteId":"outbound_rtp_video_1","ssrc":"155596793","bytesReceived":15640,"jitter":2.067,"mozRtt":6,"packetsLost":0,"packetsReceived":23},"inbound_rtp_video_1":{"id":"inbound_rtp_video_1","timestamp":1408234290403.7036,"type":"inboundrtp","bitrateMean":193105,"bitrateStdDev":155551.99720136457,"framerateMean":12.5,"framerateStdDev":10.472185381603339,"isRemote":false,"remoteId":"inbound_rtcp_video_1","ssrc":"1201038247","bytesReceived":119409,"discardedPackets":0,"jitter":2.005,"packetsLost":0,"packetsReceived":140},"outbound_rtp_video_1":{"id":"outbound_rtp_video_1","timestamp":1408234290403.7036,"type":"outboundrtp","bitrateMean":181200,"bitrateStdDev":159340.97680969158,"framerateMean":16.5,"framerateStdDev":11.387127235025815,"isRemote":false,"remoteId":"outbound_rtcp_video_1","ssrc":"155596793","bytesSent":108347,"droppedFrames":32,"packetsSent":135},"inbound_rtcp_video_1":{"id":"inbound_rtcp_video_1","timestamp":1408234289054.908,"type":"outboundrtp","isRemote":true,"remoteId":"inbound_rtp_video_1","ssrc":"1201038247","bytesSent":54847,"packetsSent":71},


And the H.264 stats:
884 INFO PeerConnectionWrapper (pcRemote): Got stats: {"inbound_rtp_video_1":{"id":"inbound_rtp_video_1","timestamp":1408234393182.202,"type":"inboundrtp","bitrateMean":0,"bitrateStdDev":0,"framerateMean":0,"framerateStdDev":0,"isRemote":false,"ssrc":"0","bytesReceived":0,"discardedPackets":0,"packetsReceived":0},"outbound_rtp_video_1":{"id":"outbound_rtp_video_1","timestamp":1408234393182.202,"type":"outboundrtp","bitrateMean":0,"bitrateStdDev":0,"framerateMean":0,"framerateStdDev":0,"isRemote":false,"remoteId":"","ssrc":"1992613993","bytesSent":0,"droppedFrames":155,"packetsSent":0},

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Make video visible in mochitests

Comment 2

[Martin Thomson [:mt:]]

Confirmed.  Also, a leak:

 2:51.11      |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
 2:51.11                                               Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
    0 TOTAL                                          19      464 17445405        3 ( 6681.80 +/- 10723.57)  7246893        0 ( 4691.04 +/-  8150.54)
   98 CondVar                                        40       40      330        1 (   36.13 +/-     7.64)        0        0 (    0.00 +/-     0.00)
  182 GeckoChildProcessHost                         392      392        1        1 (    1.00 +/-     0.00)        0        0 (    0.00 +/-     0.00)
  304 Mutex                                          32       32     1390        1 (  180.08 +/-    61.24)        0        0 (    0.00 +/-     0.00)

Comment 3

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

Attachment: Make video visible in mochitests

If you want to observe the video you decoded, you need to have it not stop immediately on time updates in the video element -- the previous patch can fail to show video for VP8 as well (and does on my linux64 debug build)

Comment 4

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

Attachment: Emulate an actual H.264 NAL sequence

It's unclear if this was an initial problem or a recent regression due to the rework of all the H.264 depacketization code by Mo Zanaty, but the depacketization code ignores unknown NAL types, which all of this appeared as.  Now emulates SPS/PPS/IDR on keyframes, and ignores SPS/PPS on decode.  No leaks when run locally.

Comment 5

[Eric Rescorla (:ekr)]

\
It's really important that(In reply to Randell Jesup [:jesup] from comment #4)
> Created attachment 8474194 [details] [diff] [review]
> Emulate an actual H.264 NAL sequence
> 
> It's unclear if this was an initial problem or a recent regression due to
> the rework of all the H.264 depacketization code by Mo Zanaty, but the
> depacketization code ignores unknown NAL types, which all of this appeared
> as.  Now emulates SPS/PPS/IDR on keyframes, and ignores SPS/PPS on decode. 
> No leaks when run locally.

It's a regression. Things worked fine a month ago.

The mochitests really need to be modified so that they catch this sort
of thing. As it is, there are essentially no automation tests that
verify that GMP works at all.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #3)
> Created attachment 8474193 [details] [diff] [review]
> Make video visible in mochitests
> 
> If you want to observe the video you decoded, you need to have it not stop
> immediately on time updates in the video element -- the previous patch can
> fail to show video for VP8 as well (and does on my linux64 debug build)

If you run the mochitest standalone using the command above it
continues to display video indefinitely.

Comment 7

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

(In reply to Eric Rescorla (:ekr) from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #3)
> > Created attachment 8474193 [details] [diff] [review]
> > Make video visible in mochitests
> > 
> > If you want to observe the video you decoded, you need to have it not stop
> > immediately on time updates in the video element -- the previous patch can
> > fail to show video for VP8 as well (and does on my linux64 debug build)
> 
> If you run the mochitest standalone using the command above it
> continues to display video indefinitely.

I believe after 10 seconds it decides media has succeeded and ends the call (and you'll note remote video stops changing color) and shows PASSED

Comment 8

[Nils Ohlmeier [:drno]]

(In reply to Randell Jesup [:jesup] from comment #7)
> I believe after 10 seconds it decides media has succeeded and ends the call
> (and you'll note remote video stops changing color) and shows PASSED

Unfortunately the whole media detection is a bad joke (for audio and video), because if you parse the logs manually you will see the events even firing if no ICE connection got established (yet).
But you are right once the current tests decide everything has passed the peer connections get closed, which stops the media rendering on the receiving side (the local previews continue until you close the browser).

Comment 9

[Nils Ohlmeier [:drno]]

Attachment: bug_1054704_rtcp_verification.patch

This patch actually verifies the RTCP stats, like we do already for the 3 hour calls in the QA lab. But this needs a little re-work before we can make it part of all tests as the sheriffs would probably not like all of our PC tests to wait for 30 seconds.

Comment 10

[Nils Ohlmeier [:drno]]

For me with my patch applied, I see actually the fake plugin crashing right away and no video getting rendered at all.
Unfortunately mochitest detects that plugin crash only after the test has ended. Seems we should check that somehow during the call.

Comment 11

[Nils Ohlmeier [:drno]]

On 33 I actually see RTP being send and received (according to RTCP stats), no crash report (I'm guessing because plugin crash reports are still de-activated on Auraora?), but still nothing getting rendered on the receiving end.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Eric Rescorla (:ekr) from comment #5)
> It's a regression. Things worked fine a month ago.

A regression range might be enlightening here.

Comment 13

[Eric Rescorla (:ekr)]

Somewhere between these two git commits:

f70ff48744a68bfa6396d90f795329a4c6e45908
..
171665617d546427cd1b48a4f90ee13f6f1e19b4

Comment 14

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #7)
> (In reply to Eric Rescorla (:ekr) from comment #6)
> > (In reply to Randell Jesup [:jesup] from comment #3)
> > > Created attachment 8474193 [details] [diff] [review]
> > > Make video visible in mochitests
> > > 
> > > If you want to observe the video you decoded, you need to have it not stop
> > > immediately on time updates in the video element -- the previous patch can
> > > fail to show video for VP8 as well (and does on my linux64 debug build)
> > 
> > If you run the mochitest standalone using the command above it
> > continues to display video indefinitely.
> 
> I believe after 10 seconds it decides media has succeeded and ends the call
> (and you'll note remote video stops changing color) and shows PASSED


If this test hasn't passed in 10 seconds, that should probably be marked
as a failure.

Comment 15

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

(In reply to Eric Rescorla (:ekr) from comment #14)
> If this test hasn't passed in 10 seconds, that should probably be marked
> as a failure.

I was just commenting on what you see in a manual test with these patches; testing for actual video rendering will be covered in bug 1054706, which I just attached a patch to do that (without waiting 10 seconds).

Comment 16

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

(In reply to Eric Rescorla (:ekr) from comment #13)
> Somewhere between these two git commits:
> 
> f70ff48744a68bfa6396d90f795329a4c6e45908
> ..
> 171665617d546427cd1b48a4f90ee13f6f1e19b4

For those of us that don't keep a mozilla git around; can you tell me what hg comment (or bug #'s) those map to?  I'd hope that info's available

Comment 17

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #16)
> (In reply to Eric Rescorla (:ekr) from comment #13)
> > Somewhere between these two git commits:
> > 
> > f70ff48744a68bfa6396d90f795329a4c6e45908
> > ..
> > 171665617d546427cd1b48a4f90ee13f6f1e19b4
> 
> For those of us that don't keep a mozilla git around; can you tell me what
> hg comment (or bug #'s) those map to?  I'd hope that info's available

They're just random commits I picked by day.

You can find git commits at:
https://github.com/mozilla/gecko-dev/commit/<commit-id>

Comment 18

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

(In reply to Nils Ohlmeier [:drno] from comment #11)
> On 33 I actually see RTP being send and received (according to RTCP stats),
> no crash report (I'm guessing because plugin crash reports are still
> de-activated on Auraora?), but still nothing getting rendered on the
> receiving end.

Plugin Crash Reporting works on an Aurora build (just checked it); perhaps there's some piece that links that to mochitests that wasn't uplifted.  (Georg?)

Comment 19

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

(In reply to Eric Rescorla (:ekr) from comment #17)
> (In reply to Randell Jesup [:jesup] from comment #16)
> > (In reply to Eric Rescorla (:ekr) from comment #13)
> > > Somewhere between these two git commits:
> > > 
> > > f70ff48744a68bfa6396d90f795329a4c6e45908
> > > ..
> > > 171665617d546427cd1b48a4f90ee13f6f1e19b4
> > 
> > For those of us that don't keep a mozilla git around; can you tell me what
> > hg comment (or bug #'s) those map to?  I'd hope that info's available
> 
> They're just random commits I picked by day.

So roughly 10 days ago to 2 days ago.  99% certain it was Mo's patch; perhaps the increased reliance on the Marker bit (likely it was a bit of luck it worked before).  But it really doesn't matter; the plugin didn't produce something that smelled like a valid H.264 stream and now it does with this patch.

Comment 20

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #19)
> (In reply to Eric Rescorla (:ekr) from comment #17)
> > (In reply to Randell Jesup [:jesup] from comment #16)
> > > (In reply to Eric Rescorla (:ekr) from comment #13)
> > > > Somewhere between these two git commits:
> > > > 
> > > > f70ff48744a68bfa6396d90f795329a4c6e45908
> > > > ..
> > > > 171665617d546427cd1b48a4f90ee13f6f1e19b4
> > > 
> > > For those of us that don't keep a mozilla git around; can you tell me what
> > > hg comment (or bug #'s) those map to?  I'd hope that info's available
> > 
> > They're just random commits I picked by day.
> 
> So roughly 10 days ago to 2 days ago.  99% certain it was Mo's patch;
> perhaps the increased reliance on the Marker bit (likely it was a bit of
> luck it worked before).  But it really doesn't matter; the plugin didn't
> produce something that smelled like a valid H.264 stream and now it does
> with this patch.

It wasn't luck. I tested it and fiddled with the bitstream until it worked.

Any changes to these code paths need to be tested to verify that they don't
break the fake plugin. Until your patch in bug 1054706 lands, that means
manual verification in calls.

Comment 21

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

(In reply to Eric Rescorla (:ekr) from comment #20)

> It wasn't luck. I tested it and fiddled with the bitstream until it worked.

It worked, but as mentioned it was chance - the bitstream looked nothing like a real H.264 stream, and so changes that assumed you have an h.264 stream broke it.  However, it really doesn't matter.

> Any changes to these code paths need to be tested to verify that they don't
> break the fake plugin. Until your patch in bug 1054706 lands, that means
> manual verification in calls.

Sure, but I don't anticipate any such changes - and we have a patch for that bug.  So I suspect this will all be moot tomorrow.

Comment 22

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

Attachment: Make video visible in mochitests

applies on top of bug 1054706

Comment 23

[Georg Fritzsche [:gfritzsche]]

(In reply to Randell Jesup [:jesup] from comment #18)
> (In reply to Nils Ohlmeier [:drno] from comment #11)
> > On 33 I actually see RTP being send and received (according to RTCP stats),
> > no crash report (I'm guessing because plugin crash reports are still
> > de-activated on Auraora?), but still nothing getting rendered on the
> > receiving end.
> 
> Plugin Crash Reporting works on an Aurora build (just checked it); perhaps
> there's some piece that links that to mochitests that wasn't uplifted. 
> (Georg?)

I don't quite understand the question - do you want crash reports from mochitests? We don't keep them around there (and mochitests use separate profiles anyway).

Comment 24

[Ethan Hugg [:ehugg]]

Comment on attachment 8474194 [details] [diff] [review]
Emulate an actual H.264 NAL sequence

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

This worked for me on Linux, but on OSX it was unable to load the fake plugin (it will say plugin crashed).  

To fix this I changed the libfake.dylib in obj/dist/NightlyDebug.app/Contents/MacOS/gmp-fake from a symlink to a copy of the dylib and then it works.  For some reason it has trouble reading this symlink when opening the libfake.dylib on OSX so perhaps we should copy it instead.

::: dom/media/gmp-plugin/gmp-fake.cpp
@@ +97,5 @@
>    uint8_t v_;
>    uint32_t timestamp_;
>  };
>  
> +// Must match frst fields of EncodedFrame

"first"?

Comment 25

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

ted - how can we do that for mac?  I suspect this was already broken (and Nils has reported something very like this on Mac).  I suspect this works in automation/tbpl.
https://tbpl.mozilla.org/?tree=Try&rev=e560e77ff455

Comment 26

[Eric Rescorla (:ekr)]

Attachment: Change fake NAL type

Comment 27

[Eric Rescorla (:ekr)]

The patch in c26 is the minimal fix here. It simply claims everything is a key
frame (which is true for this encoder). I suggest that we land the minimal
patch and rely on the tests in 1054706 to catch any changes that introduce
new dependencies.

Comment 28

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

Comment on attachment 8475356 [details] [diff] [review]
Change fake NAL type

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

My preference would be for something that better approximated a real stream so as to test the packetization code properly (remember, this tests H.264 packetization codepaths as well, not just the GMP interface) - but this should work.

::: dom/media/gmp-plugin/gmp-fake.cpp
@@ +172,5 @@
>  
>      // Copy the data. This really should convert this to network byte order.
>      EncodedFrame eframe;
>      eframe.length_ = sizeof(eframe) - sizeof(uint32_t);
> +    eframe.h264_compat_ = 5;

Comment that this is an H.264 IDR NAL type

Comment that we're not sending SPS/PPS

Comment 29

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

To be clear, my ideal fake plugin would generate SPS/PPS, would generate encoded data that approximated (with random noise) the bitrate requested, and generated multiple NALs if the data doesn't fit in the max encoded NAL size passed in.

Comment 30

[Martin Thomson [:mt:]]

(In reply to Randell Jesup [:jesup] from comment #29)
> To be clear, my ideal fake plugin would generate SPS/PPS, would generate
> encoded data that approximated (with random noise) the bitrate requested,
> and generated multiple NALs if the data doesn't fit in the max encoded NAL
> size passed in.

Maybe you can express your preferences in the form of a bug, so we can track it.

Comment 31

[Eric Rescorla (:ekr)]

Attachment: Generate an IDR NAL for fake H.264

Comment 32

[Eric Rescorla (:ekr)]

Comment on attachment 8476787 [details] [diff] [review]
Generate an IDR NAL for fake H.264

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

Carrying forward r+ from Jesup.

Comment 33

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=474e987213bb

https://hg.mozilla.org/integration/mozilla-inbound/rev/bead3ac993fa

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bead3ac993fa

Comment 35

[(not currently active) Ted Mielczarek]

(In reply to Randell Jesup [:jesup] from comment #25)
> ted - how can we do that for mac?  I suspect this was already broken (and
> Nils has reported something very like this on Mac).  I suspect this works in
> automation/tbpl.
> https://tbpl.mozilla.org/?tree=Try&rev=e560e77ff455

I believe we discussed this on IRC and smichaud has a fix somewhere. Re-needinfo me if my undestanding is incorrect.

Comment 36

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

Comment on attachment 8476787 [details] [diff] [review]
Generate an IDR NAL for fake H.264

Approval Request Comment
[Feature/regressing bug #]: GMP/OpenH264

[User impact if declined]: tests don't work

[Describe test coverage new/current, TBPL]: This is part of the test coverage for GMP

[Risks and why]: no risk, test only.  Landed on inbound.

[String/UUID change made/needed]: none

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84bdca67fffe

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Should have been:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e276d1a68b63