1411681 - H.264 High 4.0 profile streams no longer display when using WebRTC (regression)

Status: RESOLVED FIXED

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

Description

[Joe Manojlovich]

Attachment: aboutWebrtc.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

Upgrade to Firefox 56 or greater. Attempt to view a network camera stream from a WebRTC server. The camera uses the H.264 High 4.0 profile (profile/level ID 0x640028). 

Mozregression output ends with:

2017-10-25T11:26:55: INFO : Narrowed inbound regression window from [c7428449, 93c502ef] (3 builds) to [c7428449, 5bbdb7d3] (2 builds) (~1 steps left)
2017-10-25T11:26:55: DEBUG : Starting merge handling...
2017-10-25T11:26:55: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e&full=1
2017-10-25T11:26:56: DEBUG : Found commit message:
Bug 1361703: enable NR_epoll_create/create1 in linux sandbox r=jld
Enables creating new libevent epoll queues on Linux


In the [c7428449, 5bbdb7d3] window is a likely culprit: https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb06ea384e9

Attaching an about:webrtc capture as well.


Actual results:

The firefox WebRTC subsystem correctly negotiates a connection with the WebRTC server. The camera data is being received by the browser. No video is being rendered.


Expected results:

The H.264 High 4.0 profile camera stream should render, as it did with Firefox 55.

Comment 1

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

[Tracking Requested - why for this release]: regression

Nils, can you assess the damage here?

Comment 2

[Nils Ohlmeier [:drno]]

Joe, can you please double check what versions of the OpenH264 extension you have installed?

I'm asking because we are in the process of rolling out OpenH264 1.7.1. On Firefox Nightly 58 and on Firefox Beta 57 you might have the new version 1.7.1 already. But on Firefox Release 56 you still should have the OpenH264 extension version 1.6.

You can check the version by opening 'about:addons' in a new tab and then clicking on 'Extensions' on the left hand side. And then click on 'More' in the OpenH264 row.

Because the roll out of the extension is independent of the Firefox binaries it mozregression could easily return you strange results. So it would be a big help if you could double check the OpenH264 versions numbers you see. I'm assuming based on the data you provided so far that it's broken OpenH264 1.6 and 1.7.1.

Comment 3

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

Both offer and answer in the about:webrtc you provided used profile-level-id=42e01f:

a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1

If the video is actually encoded in 6400xx, then your negotiation is incorrect.

In any case, 6400xx does not match 42e01f (or any constrained baseline; Mozilla does not support High profile).  If I force it to process an SDP with profile-level-id=640028, it rejects video, correctly.  I tested, 55.0.3 does the same thing.

Can you provide an about:webrtc from your case before 56?  Are you negotiating constrained-baseline level 3.1, but actually sending High profile level 4.0?  If so, that's not valid, and it shouldn't have worked before.


OpenH264 may support enough of High Profile to work with the camera... but that's different than full High Profile support.  1.5.0 and later support "CHP" for decode only; we don't expose that partial support.  see https://github.com/cisco/openh264/blob/master/RELEASES

Also note that CHP is not a recognized H264 standard profile; it can't be specified exactly in the SDP

Probably (with faked CBP negotiation) High from this camera was decodable, but the update to the H264 packetization code added some checking or in some other way broke receiving High profile masquerading as Constrained Baseline

<Removing Tracking, moving back to Unconfirmed until we get more info>

Comment 4

[Joe Manojlovich]

(In reply to Nils Ohlmeier [:drno] from comment #2)
> Joe, can you please double check what versions of the OpenH264 extension you
> have installed?
> 
> I'm asking because we are in the process of rolling out OpenH264 1.7.1. On
> Firefox Nightly 58 and on Firefox Beta 57 you might have the new version
> 1.7.1 already. But on Firefox Release 56 you still should have the OpenH264
> extension version 1.6.
> 
> You can check the version by opening 'about:addons' in a new tab and then
> clicking on 'Extensions' on the left hand side. And then click on 'More' in
> the OpenH264 row.
> 
> Because the roll out of the extension is independent of the Firefox binaries
> it mozregression could easily return you strange results. So it would be a
> big help if you could double check the OpenH264 versions numbers you see.
> I'm assuming based on the data you provided so far that it's broken OpenH264
> 1.6 and 1.7.1.

For my latest 2017-10-25 Nightly, which is broken too, it's 1.7.1

For my Firefox 56 it's 1.6

Comment 5

[Joe Manojlovich]

Attachment: aboutWebrtc55.html

Here's the about:webrtc from version 55

Comment 6

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

(In reply to Joe Manojlovich from comment #5)
> Created attachment 8922351 [details]
> aboutWebrtc55.html
> 
> Here's the about:webrtc from version 55

So it has always been lying about the profile.  OpenH264 can decode *some* subsets of High profile, but not general high profile.  

I suspect strongly that some of the packetization code changed and started blocking something in your stream.

I'm not sure we would 'fix' this if we find the cause, but logs taken with these env vars may help:

MOZ_LOG=timestamp,webrtc_trace:5,signaling:5,jsep:5,rtplogger:5
WEBRTC_TRACE_FILE=nspr
MOZ_LOG_FILE=<some temp directory>

See also https://blog.mozilla.org/webrtc/debugging-encrypted-rtp-is-more-fun-than-it-used-to-be/
(Note that only a few bytes of the payload will be included in the rtp logs/pcap generated from it)

The log will be large; try not to let it go too long.  compressed it may be small enough to go here, otherwise send a link by email to a dropbox/etc location.

Comment 7

[Joe Manojlovich]

Attachment: log.zip

Log file created using the following environment variables:

MOZ_LOG=timestamp,webrtc_trace:5,signaling:5,jsep:5,rtplogger:5
WEBRTC_TRACE_FILE=nspr
MOZ_LOG_FILE=%TEMP%

Comment 8

[[e-mail]]

No doubt this is a new problem.  For years (1.5 at least) High was supported... I used a network camera that worked excellently.  Can't tell you how frustrating it is that it just STOPPED working out of the blue.  

Google did something similar back in April.  They just removed all support for anything but constrained baseline.  This was after 1 year of supporting all of the basic H264 varieties (which their codec clearly can support).  Their reason is that they want people to use their VP8 codec.  What is firefox's reason?  Did someone from google get into the code base? :)

Comment 9

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

Cliff:
to begin with, we share with google the open-source code from webrtc.org (which was open-sourced by google, and they're the primary developers working on it - originally that was code from GIPS, and the original GIPS team formed the core of their webrtc team).  So we do import code from google occasionally, and that's what happened here.  We do make mods to the upstream code, and at times we've run with considerable patches, in particular for H.264 before Chrome started supporting it.  (The webrtc.org code had H.264 code, but it was a bit buggy originally and Google wasn't working on it.)

Your camera is lying, however, and violating the SDP spec.  The browser said "I want to receive constrained baseline", and your camera agreed to that, but then didn't send what it agreed to.  I'm an author of RFC 6184 (the packetization/SDP spec for H.264), and the camera is clearly violating it.  The other part was that OpenH264 happened to decode it anyways, since it supports decoding (only) *some* of High, but I don't think the subset of High it supports matches any profile-level-id that could be offered.  So the browser can't say "I can receive High" because then someone may send it *real* high, and it can't decode that.  So we offer constrained baseline.  (we could offer plain baseline as well I believe, but there's little or no advantage to plain baseline over constrained baseline in practice.)

I'll glance at the log, since that might indicate where the H264 packetization code is getting upset about the High (I suspect strongly while parsing PPS/SPS NAL's, since nothing elsewhere is likely to be noticeably different to anything but the decoder).

Comment 10

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

This is probably the problem:
2017-10-26 20:26:04.738000 UTC - [Socket Thread]: V/signaling [Socket Thread|WebrtcVideoSessionConduit] VideoConduit.cpp:2116: mozilla::WebrtcVideoConduit::ReceivedRTPPacket: seq# 412, Len 244, SSRC 989089316 (0x3af44e24) 
2017-10-26 20:26:04.738000 UTC - [Socket Thread]: D/webrtc_trace (sps_parser.cc:111): SPS contains scaling lists, which are unsupported.
2017-10-26 20:26:04.738000 UTC - [Socket Thread]: D/webrtc_trace (rtp_format_h264.cc:514): Failed to parse SPS id from SPS slice.
2017-10-26 20:26:04.738000 UTC - [Socket Thread]: D/webrtc_trace (sps_parser.cc:111): SPS contains scaling lists, which are unsupported.
2017-10-26 20:26:04.738000 UTC - [Socket Thread]: D/webrtc_trace (rtp_format_h264.cc:514): Failed to parse SPS id from SPS slice.

sps_parser.cc:
      // We don't support reading the sequence scaling list, and we don't really
      // see/use them in practice, so we'll just reject the full sps if we see
      // any provided.
      if (seq_scaling_list_present_flags > 0) {
        LOG(LS_WARNING) << "SPS contains scaling lists, which are unsupported.";
        return OptionalSps();
      }

Since the SPS data is mostly just being parsed and ignored other than resolution, a patch for that code block might resolve the issue.  You could report it upstream, though I doubt they would be interested in fixing it for this sort of spec-violating usecase.  However, if you provide a patch, they might take it.  (and if you give us a patch, we might land it - we do carry mods to the upstream.)  It might be very simple to ignore them.

Comment 11

[Joe Manojlovich]

I can confirm that commenting out the return statement in the code block above fixes the issue for me.

Comment 12

[Joe Manojlovich]

Attachment: sps_parser.patch

Comment 13

[[e-mail]]

So it appears then that the code above was added to webrtc code base in order to toss ALL video that uses scaling lists?  Excuse my naiveté when it comes to the H.264 protocol.  Given that this used to work it seems that it is removing functionality on some browsers that some people have been counting on for 1-2 years.  Perhaps that kind of check would be better added at the browser level (down stream).  Also, regarding the spec not specifically supporting High/Constrained High profiles:  Does it specifically say you CAN NOT support it?  I would think not since even the guys at chrome claim they are working on integrating High profiles:

from https://groups.google.com/forum/#!topic/discuss-webrtc/EKLBlSaiYdU:

"Chrome only supports Constrained Baseline Profile right now. We are working on adding support for more profiles, like (unconstrained) Baseline Profile, Main Profile, High Profile, etc. We didn't look at the SDP previously and accepted any profile, and that might work depending on what H264 decoder you end up with (SW or one of the HW decoders). Since we now look at the profile-level-id, we reject anything else than Constrained Baseline Profile. We are working on adding support for more profiles, but we need to properly enumerate the capabilities of the H264 decoder before accepting other profiles. You can work around the new profile enforcement in Chrome by setting profile-level-id to 42E01F if you want to get the old behavior back, but it's undefined behavior and we can't guarantee it will work."

So it seems that it would be better if the support for this type of thing was handled at the browser level (down stream)  and not the webrtc codebase.... Am I wrong there?

Comment 14

[[e-mail]]

Guys

i found this bug report on webrtc.org:

https://bugs.chromium.org/p/webrtc/issues/detail?id=8275&q=seq_scaling_list_present_flag&colspec=ID%20Pri%20M%20ReleaseBlock%20Component%20Status%20Owner%20Summary

This FIXES the problem (I verified it).  HOWEVER I pulled the latest main webrtc tree and I don't see the changes.. Maybe there is a beta webrtc tree I am missing?

Comment 15

[Joe Manojlovich]

The chromium patch in comment 14 works for me as well.

Comment 16

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

gerv: the patch from chromium.org (for the webrtc.org codebase, actually) has not apparently landed or been submitted as a CL upstream.  What can we do/not do here?  Could we get the author to submit the patch to us, and what would we need him to do in conjunction with such a patch?

This is a patch to code we import from webrtc.org, which is under a BSD license.

cliff: if this lands upstream, it's very easy for us to "cherry-pick" the fix and get it into 58 (if it lands upstream *soon*, like this week).

Comment 17

[Gervase Markham [:gerv]]

It's clear from the bug report that the patch author had intent to submit it, and therefore intent to make it available under webrtc.org licensing terms. So we can just take it if we want it; we don't need him to submit it to us specifically.

Gerv

Comment 18

[[e-mail]]

Guys

I have been in discussion with the author of the patch (Marc L.) and he says he is fine with someone else submitting his patch.  He encountered a hiccup during the submission process and that is why it was never committed.  

Can someone here commit this patch to webrtc.org or should I?  I am not familiar with the submission process so I am kind of hoping someone else will. ;) ;)

Once again this code snippet is courtesy of Marc L. (I don't want to include his full name for privacy) and I thank him whole heartedly.  Maybe he can reply to this chain and take a bow for this piece of code lol.  


HERE IS THE CODE:


    if (seq_scaling_matrix_present_flag) {
      //process the scaling lists just enough to be able to properly skip over them
      //so we can still read the resolution on streams where this is included.
      for(int i = 0; i < (3 != chroma_format_idc ? 8 : 12); i++){
        //seq_scaling_list_present_flag[i]  : u(1)
        uint32_t seq_scaling_list_present_flags;
        RETURN_EMPTY_ON_FAIL(buffer->ReadBits(&seq_scaling_list_present_flags, 1));
        if (seq_scaling_list_present_flags > 0) {
          int last_scale = 8;
          int next_scale = 8;
          int size_of_scaling_list = i < 6 ? 16 : 64;
          for(int j = 0; j < size_of_scaling_list; j++){
            if(0 != next_scale){
              uint32_t delta_scale;
              //delta_scale: se(v)
              RETURN_EMPTY_ON_FAIL(buffer->ReadExponentialGolomb(&delta_scale));
              next_scale = (last_scale + delta_scale + 256) % 256;
            }
            last_scale = (0 == next_scale) ? last_scale : next_scale;
          }
        }
      }
    }

Comment 19

[Eric Rescorla (:ekr)]

Well, if it is going to go into webrtc.org, it probably needs execution of the CLA

Comment 20

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

From what Gerv says, we can probably take the patch locally.  However, I don't know that we can commit it upstream into webrtc.org (or more to the point file a CL with it) without the patch author filling out a CLA with webrtc.org aka Google.  

Gerv: if a patch is to BSD-licensed code from a contributor (though not one who's signed an agreement with Mozilla), and we land it in our tree, can we submit it upstream under our CLA with Google? 

The simplest thing would be for the patch writer to submit it to Google/webrtc.org after filling out their CLA.  Second-simplest would be to submit the patch to us directly under our requirements, in particular sections #2 and #3 of https://www.mozilla.org/en-US/about/governance/policies/commit/requirements/

Under #4 there we can commit code written by others.  We do know from the Chromium bug at least the email address of the author, and per Gerv's previous comment it's clear they intended it to be under the BSD license of the code he was modifying.  So we should be able to commit it locally.  Submitting it upstream after we land it locally may be trickier, though we could just say "this is a copy of the patch from bug NNNN"

Comment 21

[Gervase Markham [:gerv]]

Hard to say. I suspect the point of the CLA is to get the assent of the copyright owner, although as it's not a reciprocal licence, I have no idea why they need one at all. I think it would certainly be easiest if we got the upstream person to sign the CLA; once they did, we can certainly submit it with his name on, doing the mechanics for him. If he won't sign the CLA or is unresponsive, we'll need to ask legal.

None of these things, of course, prevents us from taking and carrying the patch ourselves.

Gerv

Comment 22

[[e-mail]]

Guys

I have an email from Marc (the original code author) saying I could submit the code and even "snag credit for the change" (which I wouldn't do obviously).  I submitted the CLA on the webrtc site.

Shall I also submit the patch (Do I have permissions for that as a normal user?)??

Cliff

Comment 23

[Gervase Markham [:gerv]]

(In reply to [e-mail] from comment #22) 
> Shall I also submit the patch (Do I have permissions for that as a normal
> user?)??

Whether it's acceptable for you to submit this patch you did not write on the author's behalf is a question for webrtc.org, not for me.

Gerv

Comment 24

[[e-mail]]

OK I will commit it.  I have permission (in writing) to take ownership of the code and will do the CLA and submit the patch.

In the meantime I think we are good to go as far as patching firefox... right?

Cliff

Comment 25

[Gervase Markham [:gerv]]

Are you asking me? Comment #21 has my answer.

Gerv

Comment 26

[[e-mail]]

Yes I noticed you said that none of this prevents you guys from carrying the  patch.  I assume this means it will become part of firefox.  I will submit it to webrtc.org when I get a chance.  

Thanks for the help,

Cliff

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Cliff, was your patch accepted by WebRTC?   Do you still see the issue or is it now fixed?

Comment 28

[Alexander Gordeev]

Hi!
The discussed patch was accepted by WebRTC SDK and was released in Chrome M65 in March. Unfortunately, it didn't yet land in firefox. Just checked the dev tree. Can you please pull it from them?
I have the issue when streaming from one of my cameras.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Jan, what do you think? Is this something we may now want to pull into Firefox?

Comment 30

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

Dan, what are our options?

Comment 31

[Dan Minor [:dminor]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #30)
> Dan, what are our options?

I think this is the patch in question [1]. It is small and looks like it should apply cleanly. I don't think there's any harm in taking it, we'll get it with the next update anyway.

[1] https://webrtc.googlesource.com/src.git/+/6728003bcf5657da8f9ed52ad84f46c1197ce54b%5E%21/

Comment 32

[Alexander Gordeev]

Thanks! Yes, this is the right patch, Dan. Can you please give an estimate of when it will happen? I know that WebRTC SDK is a fast moving target that can be really hard to follow. I see from the gecko-dev repo history that it can take more then a year until the next update. If this is the current estimate then maybe pulling just this patch is an option? It looks really harmless indeed.

Comment 33

[Alexander Gordeev]

Ah, sorry, didn't get that you're already going to take it :)

Comment 34

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

Sgtm.

Comment 35

[Dan Minor [:dminor]]

Attachment: Bug 1411681 - Cherrypick 6728003bcf5657da8f9ed52ad84f46c1197ce54b from upstream webrtc.org; r=pehrsons!

Comment 36

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd56311d18ac14c079c88182e18955794e010914

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de0a29776dee
Cherrypick 6728003bcf5657da8f9ed52ad84f46c1197ce54b from upstream webrtc.org; r=pehrsons

Comment 38

[Arthur Iakab [arthur_iakab]]

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

Comment 39

[Arthur Iakab [arthur_iakab]]

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

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Is this worth backporting to Beta for 64 as well? If so, please nominate it for approval.

Comment 41

[Dan Minor [:dminor]]

Comment on attachment 9026351 [details]
Bug 1411681 - Cherrypick 6728003bcf5657da8f9ed52ad84f46c1197ce54b from upstream webrtc.org; r=pehrsons!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1341285

User impact if declined: Broken video for H.264 High 4.0 profile camera streams.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a backport from upstream webrtc.org. The patch there landed back in January 2018, so this should be safe.

String changes made/needed: None

Comment 42

[Dan Minor [:dminor]]

It looks like this will apply to beta without any changes.

Alexander, we're considering taking this for beta. If you have time, it would be great if you could verify that this change fixed the problem for you. Thanks!

Comment 43

[Alexander Gordeev]

(In reply to Dan Minor [:dminor] from comment #42)
> It looks like this will apply to beta without any changes.
> 
> Alexander, we're considering taking this for beta. If you have time, it
> would be great if you could verify that this change fixed the problem for
> you. Thanks!

Thank you!
Sure, I'd be happy to do that. However, I'm not familiar with the process. Is it already in the build here https://www.mozilla.org/en-US/firefox/channel/desktop/ ?

Comment 44

[Dan Minor [:dminor]]

(In reply to Alexander Gordeev from comment #43)
> (In reply to Dan Minor [:dminor] from comment #42)
> > It looks like this will apply to beta without any changes.
> > 
> > Alexander, we're considering taking this for beta. If you have time, it
> > would be great if you could verify that this change fixed the problem for
> > you. Thanks!
> 
> Thank you!
> Sure, I'd be happy to do that. However, I'm not familiar with the process.
> Is it already in the build here
> https://www.mozilla.org/en-US/firefox/channel/desktop/ ?

Yes, grab the "Nightly" build from there and see if it fixes the problem for you. Thanks for having a look!

Comment 45

[Alexander Gordeev]

Just checked the nightly build. I can see the video now. Also there are no SPS parsing related errors in the log. Thanks!

Comment 46

[Dan Minor [:dminor]]

(In reply to Alexander Gordeev from comment #45)
> Just checked the nightly build. I can see the video now. Also there are no
> SPS parsing related errors in the log. Thanks!

Thank you for checking this out!

Comment 47

[Julien Cristau [:jcristau]]

AFAICT this is an old issue, has no duplicates, and we're at the end of beta64, so I'd rather let it ride to 65.