Fake H.264 encoder doesn't work in mochitests

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ekr, Assigned: ekr)

Tracking

(Blocks 1 bug, {regression})

33 Branch
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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},
Assignee: nobody → ekr
Status: NEW → ASSIGNED
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)
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)
Attachment #8474164 - Attachment is obsolete: true
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.
Attachment #8474194 - Flags: review?(ethanhugg)
Whiteboard: [openh264-uplift]
\
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.
(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.
(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
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
(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).
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.
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.
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.
(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.
Somewhere between these two git commits:

f70ff48744a68bfa6396d90f795329a4c6e45908
..
171665617d546427cd1b48a4f90ee13f6f1e19b4
(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.
(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).
(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
(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>
(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?)
Flags: needinfo?(georg.fritzsche)
(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.

(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.
(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.
Attachment #8474193 - Attachment is obsolete: true
(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).
Flags: needinfo?(georg.fritzsche)
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"?
Attachment #8474194 - Flags: review?(ethanhugg) → review+
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
Flags: needinfo?(ted)
Posted patch Change fake NAL type (obsolete) — Splinter Review
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 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
Attachment #8475356 - Flags: review+
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.
Blocks: OpenH264
(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 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.
Attachment #8476787 - Flags: review+
Attachment #8475356 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/bead3ac993fa
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(ted)
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
Attachment #8476787 - Flags: approval-mozilla-aurora?
Attachment #8476787 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [openh264-uplift]
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.