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

NEW
Unassigned

Status

()

Core
WebRTC: Audio/Video
P5
normal
Rank:
45
26 days ago
5 days ago

People

(Reporter: Joe Manojlovich, Unassigned)

Tracking

({regression})

Trunk
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 wontfix, firefox58 affected)

Details

(Whiteboard: [need info reporter 2017-10-25])

Attachments

(4 attachments)

(Reporter)

Description

26 days ago
Created attachment 8922008 [details]
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.

Updated

25 days ago
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
[Tracking Requested - why for this release]: regression

Nils, can you assess the damage here?
Blocks: 1341285
Status: UNCONFIRMED → NEW
Rank: 11
status-firefox56: --- → wontfix
status-firefox57: --- → fix-optional
status-firefox58: --- → affected
tracking-firefox58: --- → ?
Ever confirmed: true
Flags: needinfo?(drno)
Priority: -- → P2
Summary: H.264 High 4.0 profile streams no longer display when using WebRTC → H.264 High 4.0 profile streams no longer display when using WebRTC (regression)
Has Regression Range: --- → yes
Keywords: regression
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.
Flags: needinfo?(drno) → needinfo?(joe.manojlovich)
Whiteboard: [need info reporter 2017-10-25]
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>
Status: NEW → UNCONFIRMED
Rank: 11 → 19
status-firefox56: wontfix → ?
status-firefox57: fix-optional → ?
status-firefox58: affected → ?
tracking-firefox58: ? → ---
Ever confirmed: false
(Reporter)

Comment 4

25 days ago
(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
Flags: needinfo?(joe.manojlovich)
(Reporter)

Comment 5

25 days ago
Created attachment 8922351 [details]
aboutWebrtc55.html

Here's the about:webrtc from version 55
(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.
Flags: needinfo?(joe.manojlovich)
(Reporter)

Comment 7

24 days ago
Created attachment 8922488 [details]
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%
Flags: needinfo?(joe.manojlovich)
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? :)
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).
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.

Updated

13 days ago
Status: UNCONFIRMED → NEW
Rank: 19 → 45
status-firefox56: ? → wontfix
status-firefox57: ? → wontfix
status-firefox58: ? → affected
status-firefox-esr52: --- → unaffected
Ever confirmed: true
Priority: P2 → P5
(Reporter)

Comment 11

12 days ago
I can confirm that commenting out the return statement in the code block above fixes the issue for me.
(Reporter)

Comment 12

12 days ago
Created attachment 8926059 [details] [diff] [review]
sps_parser.patch
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?
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?
(Reporter)

Comment 15

12 days ago
The chromium patch in comment 14 works for me as well.
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).
Flags: needinfo?(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
Flags: needinfo?(gerv)
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;
          }
        }
      }
    }
Well, if it is going to go into webrtc.org, it probably needs execution of the CLA
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"
Flags: needinfo?(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
Flags: needinfo?(gerv)
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
(In reply to cliff.olmstead@gmail.com 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
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
Are you asking me? Comment #21 has my answer.

Gerv
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
You need to log in before you can comment on or make changes to this bug.