H264 streams with no IDR fail to play with MSE.

VERIFIED FIXED in Firefox 56

Status

()

--
major
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: mihail.kirillov, Assigned: jya, Mentored)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla57
All
Windows
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 verified, firefox57 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.98 Safari/537.36

Steps to reproduce:

I'm trying to play HLS stream (see attached file) via hls.js (https://github.com/video-dev/hls.js).


Actual results:

Chunks are loading but it's no playback.


Expected results:

Video must be played back. This worked correctly in FF v.50 and older.
(Reporter)

Comment 1

a year ago
https://drive.google.com/file/d/0B8W7Wb7tGOLuUUJYWm5zcTlPRlE/view?usp=sharing
This link is a fragment of the archive recording of the stream.
(Reporter)

Comment 2

a year ago
http://185.74.221.212/hls/ - This link is a fragment of the archive recording of the stream.
This worked correctly in FF v.50 and older but in FF >=51 it's no playback
(Reporter)

Updated

a year ago
OS: Unspecified → Windows
Hardware: Unspecified → All
Version: 51 Branch → unspecified
(Reporter)

Updated

a year ago
Mentor: ajones
Severity: normal → major
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Version: unspecified → Trunk
Can you provide steps to reproduce?
Flags: needinfo?(mihail.kirillov)
(Reporter)

Comment 4

a year ago
Just open http://185.74.221.212/hls/
Flags: needinfo?(mihail.kirillov)
WFM in nightly. Can you still reproduce it?
Flags: needinfo?(mihail.kirillov)
Feel free to re-open this bug if it is still an issue.
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(mihail.kirillov)
Resolution: --- → INCOMPLETE
(Reporter)

Comment 7

a year ago
Still not working
(Reporter)

Updated

a year ago
Status: RESOLVED → UNCONFIRMED
Resolution: INCOMPLETE → ---
From my test with the latest nightly on Mac, Firefox can work well on Big Bunny, Costa Rica, Tears of steel, and Pipe, but not on the default URL, http://185.74.221.212/hls/data/223_1.m3u8. 

James,
Please put this on your plate.
Flags: needinfo?(jacheng)
Ok, let me take a look first.
Flags: needinfo?(jacheng)
I can reproduce it. The video can be played using gecko50 but the latest.

I will look into it.

BTW, it seems the TS file is bad-muxed since I will get some warning log.
Assignee: nobody → jacheng
This issue caused by the change

"Bug 1300296 - Do not rely on container information to determine if a h264 frame is a keyframe"

We will got a lot of log like "Frame incorrectly marked as non-keyframe"

Since the content you provide is bad-muxed with "no keyframe" labeled in the samples[1].

we cannot find the first IDR frame so the playback is stalled in WAIT_FOR_DATA state.

Our policy is to trust the NALU type per sample instead of the container's information, so we will overwrite the mKeyframe according to the right NALU type.

Could you please check the content first? 

I think this issue might mark as won't-fix unless we decided to revert the change of bug 1300296.


[1]
http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/media/libstagefright/binding/H264.cpp#808
[2]
http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/dom/media/fmp4/MP4Demuxer.cpp#456
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → WONTFIX
(Reporter)

Comment 12

a year ago
This situation is typical for live streams (especially broadcast from satellites), unlike vod, where it is possible to spend time and resources on the correction and preparation of segments.
In addition, the problem is only in Firefox, other browsers, software players, SmartTV, media consoles and any other hardware players do not consider this an error and the content playback goes smoothly and without interruption
Comment hidden (mozreview-request)
Comment on attachment 8890786 [details]
Bug-1347518 Figure out a mechanism of tolerability.

Hi jya,

According to the comment 11 and comment 12,

Do we have any chance to make some change to tolerate this situation?

I just naively add a flag to let us found the first keyframe for decoding.

Any good idea for this case?

Thank you.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 15

a year ago
Being HLS, why would bug 1300296 have any impacts? It should never be used here.


Being HLS or MP4, doesn't really matter. You need an IDR to be able to start decoding.


I guess you could just ignore the frame type and rely on the container info instead, worse case you'll get a decoding error or visual artefacts.
We mostly needed to enforce the keyframe flags for use with MSE.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #15)
> Being HLS, why would bug 1300296 have any impacts? It should never be used
> here.
> 
IIUC, hls.js transform the ts content and use MSE for playback.
> 
> Being HLS or MP4, doesn't really matter. You need an IDR to be able to start
> decoding.
> 
> 
> I guess you could just ignore the frame type and rely on the container info
> instead, worse case you'll get a decoding error or visual artefacts.
I don't get it. In bug 1300296 we change the behavior to check the sample's NALU type. How could I make 
"ignore the frame type and rely on the container info instead" happen?

Thanks.
> We mostly needed to enforce the keyframe flags for use with MSE.
Flags: needinfo?(jyavenard)
Hi Mihail,

I use Linux Firefox to play "http://185.74.221.212/hls/" and I get 

"you are using Firefox, it looks like MediaSource is not enabled,<br>please ensure the following keys are set appropriately in <b>about:config</b><br>media.mediasource.enabled=true<br>media.mediasource.mp4.enabled=true<br><b>media.mediasource.whitelist=false"

Is it intended? Mac didn't meet this error msg.
(Reporter)

Comment 18

a year ago
Hi James,

Most of our users use Windows. Windows didn't meet this error msg too.
(Assignee)

Comment 19

a year ago
Ah ok.. my bad, I thougbt this was the HLS decoder.

then no. We won't relax that rule.

As while some decoders will accept non-IDR frame for start-up (like windows), others like the Mac H264 decoder will consistently error.

This is something that needs fixing in HLS.js
My guess is that maybe try a more recent version of hls.js


Either the content needs fixing, or HLS.js needs tagging frames differently

Per MSE spec, no data will be added to the buffer unless it finds an I-frame

Guillaume, any suggestions?
Flags: needinfo?(jyavenard) → needinfo?(g.du.pontavice)

Comment 20

a year ago
Hi Jean-Yves

hls.js is already removing non-IDR frames located before the first IDR-frame.
I checked this stream and it is a bit special.
it does not contain any type 5 NALU (IDR), however the sequence looks like this:

......  AUD SEI NDR
......  AUD SEI NDR
......  AUD SEI NDR
......  AUD SEI NDR
[log] > -3707841596/-3707859596:AUD SPS PPS SEI NDR
[log] > -3707839796/-3707857796:AUD SEI NDR 
[log] > -3707848796/-3707855996:AUD SEI NDR 
[log] > -3707846996/-3707854196:AUD SEI NDR 
[log] > -3707852396/-3707852396:AUD SEI NDR 
[log] > -3707850596/-3707850596:AUD SEI NDR 
[log] > -3707845196/-3707848796:AUD SEI NDR 
[log] > -3707843396/-3707846996:AUD SEI NDR 
[log] > -3707827196/-3707845196:AUD SEI NDR 
...


hls.js will filter-out the first NDR as they can't be decoded.
if you look at the slice type (by reading the slice header) of the NDR (located after SPS PPS) you will find out that this first NDR is actually a key frame. (slice type = 2)
see detection in hls.js : https://github.com/video-dev/hls.js/blob/master/src/demux/tsdemuxer.js#L495-L515

=> it is marked as keyframe by hls.js, and thus pushed in the fmp4 and appended into the sourcebuffer but its NALu type is kept as being NDR for sake of consistency with the original stream.
Flags: needinfo?(g.du.pontavice)
(Assignee)

Comment 21

a year ago
Does HLS.js keep the sps/pps inband, and with the NDR?

The code you linked to, only consider the NDR NAL if a sps has been found. Is that a sps anywhere prior in the stream, or just within that same frame?
Flags: needinfo?(g.du.pontavice)

Comment 22

a year ago
SPS/PPS are kept in band along with the NDR
this extra slice type check is only performed if sps is preceding this NDR, but within the same frame (for opti purpose)
Flags: needinfo?(g.du.pontavice)
(Assignee)

Comment 23

a year ago
Hmmm..

I wonder for our case (avc1) if we shouldn't worry about the sps/pps and look instead in type 1 (NDR) and 5 (I-frame)
(Assignee)

Comment 24

a year ago
Actually, I don't think this is correct. A SDR with a slice type of 2, is no guarantee that it's a keyframe.
(Assignee)

Comment 25

a year ago
Here, it's only because of the presence of SEI that you can decide the I frame is a keyframe. It's a coincidence that in this stream it's all true.
To make matter worse, SEI are optional.
(Assignee)

Updated

a year ago
Assignee: jacheng → jyavenard
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
(Assignee)

Updated

a year ago
Attachment #8890786 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

a year ago
indeed, looking for SEI_RECOVERY_POINT is more meaningful... what a mess.
Comment hidden (mozreview-request)
(Assignee)

Comment 30

a year ago
(In reply to g.du.pontavice from comment #28)
> indeed, looking for SEI_RECOVERY_POINT is more meaningful... what a mess.

Yes.

Of the two streams with issues (the one attached to this bug) and the one listed at https://github.com/video-dev/hls.js/issues/1285

They now play fine with the change.

I certainly believe that assuming a non-IDR with a subtype of 2,4,7 or 9 is a keyframe is not a good test. I guess if it was also checking that first_mb_in_slice is also 0 would be marginally better

Comment 31

a year ago
yes indeed, we need to change hls.js logic and check for SEI recovery instead

Comment 32

a year ago
mozreview-review
Comment on attachment 8899534 [details]
Bug 1347518 - P2. Don't attempt to determine frame type when encrypted.

https://reviewboard.mozilla.org/r/170816/#review176074

LOL. Yes.
Attachment #8899534 - Flags: review?(cpearce) → review+

Comment 33

a year ago
mozreview-review
Comment on attachment 8899533 [details]
Bug 1347518 - P1. Use SEI recovery point to mark keyframe.

https://reviewboard.mozilla.org/r/170814/#review176312

::: media/libstagefright/binding/H264.cpp:1016
(Diff revision 1)
> +    payloadSize = 0;
> +    if (!br.CanRead8()) {
> +      return false;
> +    }
> +    tmpByte = br.ReadU8();
> +    while (tmpByte == 0xFF) {
> +      payloadSize += 255;
> +      if (!br.CanRead8()) {
> +        return false;
> +      }
> +      tmpByte = br.ReadU8();
> +    }
> +    payloadSize += tmpByte;   // this is the last byte

Isn't there some sort of readInt32 already?  or factor this out into on (used twice), though it's not strictly necessary.  More importantly, does it encode as FFFF01 -> (256 + 256 + 1) = 513???  or should that be (256<<16 + 256<<8 + 1)?

::: media/libstagefright/binding/H264.cpp:1038
(Diff revision 1)
> +      BitReader br(p, payloadSize * 8);
> +      aDest.recovery_frame_cnt = br.ReadUE();
> +      aDest.exact_match_flag = br.ReadBit();
> +      aDest.broken_link_flag = br.ReadBit();
> +      aDest.changing_slice_group_idc = br.ReadBits(2);

What happens if payloadSize isn't big enough for these reads?

::: media/libstagefright/binding/include/mp4_demuxer/H264.h:410
(Diff revision 1)
> +    recovery_frame_cnt specifies the recovery point of output pictures in output
> +    order. All decoded pictures in output order are indicated to be correct or
> +    approximately correct in content starting at the output order position of
> +    the reference picture having the frame_num equal to the frame_num of the VCL
> +    NAL units for the current access unit incremented by recovery_frame_cnt in
> +    modulo MaxFrameNum arithmetic. recovery_frame_cnt shall be in the range of 0
> +    to MaxFrameNum − 1, inclusive.

are these exact quotes from the spec?   I'm not sure that would be covered by "fair use" in copyright law
Attachment #8899533 - Flags: review?(rjesup)
(Assignee)

Comment 34

a year ago
mozreview-review-reply
Comment on attachment 8899533 [details]
Bug 1347518 - P1. Use SEI recovery point to mark keyframe.

https://reviewboard.mozilla.org/r/170814/#review176312

> Isn't there some sort of readInt32 already?  or factor this out into on (used twice), though it's not strictly necessary.  More importantly, does it encode as FFFF01 -> (256 + 256 + 1) = 513???  or should that be (256<<16 + 256<<8 + 1)?

The algorithm is exactly as per spec, as used in the h264 reference code and the ffmpeg sei parser. 
II didn't conceive such encoding

> What happens if payloadSize isn't big enough for these reads?

Then it's an error, and the marker won't be there and false will be returned. 
All reads are done via Byte reader or BitReader which is always safe.

> are these exact quotes from the spec?   I'm not sure that would be covered by "fair use" in copyright law

It's already thoroughly used in this code. So if there's aa fair use issue, it should be addressed in another code.
(Assignee)

Comment 35

a year ago
mozreview-review-reply
Comment on attachment 8899533 [details]
Bug 1347518 - P1. Use SEI recovery point to mark keyframe.

https://reviewboard.mozilla.org/r/170814/#review176312

> Then it's an error, and the marker won't be there and false will be returned. 
> All reads are done via Byte reader or BitReader which is always safe.

II could abort if the size is too small. We don't use the result anyway
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

a year ago
mozreview-review
Comment on attachment 8899533 [details]
Bug 1347518 - P1. Use SEI recovery point to mark keyframe.

https://reviewboard.mozilla.org/r/170814/#review176484

Per IRC conversations
Attachment #8899533 - Flags: review?(rjesup) → review+
(Assignee)

Comment 39

a year ago
thanks for the reviews.

Comment 40

a year ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/217de64cdcc6
P1. Use SEI recovery point to mark keyframe. r=jesup
https://hg.mozilla.org/integration/autoland/rev/dd4b954fb20a
P2. Don't attempt to determine frame type when encrypted. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/217de64cdcc6
https://hg.mozilla.org/mozilla-central/rev/dd4b954fb20a
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

a year ago
Blocks: 778617
Keywords: regression
Summary: HLS playback problem → H264 streams with no IDR fail to play with MSE.
Can this ride the trains or should we consider backporting it to Beta?
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(jyavenard)
(Assignee)

Comment 43

a year ago
I would like it in beta, but I first I was worried about potential regressions...

There's a few streams where it introduce some decoding artifacts, but when I compared with safari they had the same problem. so this is okay...

it does make seeking in those esoteric streams much faster.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 44

a year ago
Comment on attachment 8899534 [details]
Bug 1347518 - P2. Don't attempt to determine frame type when encrypted.

Approval Request Comment
[Feature/Bug causing the regression]: 1300296
[User impact if declined]: encrypted (EME) frames will be incorrectly tagged, that could lead to decoding errors or inability to seek. 
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: it's a problem noticed via code review.
[Needs manual test from QE? If yes, steps to reproduce]:  
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: it can only prevent a problem, not create one.
[String changes made/needed]: none
Attachment #8899534 - Flags: approval-mozilla-beta?
(Assignee)

Comment 45

a year ago
Comment on attachment 8899533 [details]
Bug 1347518 - P1. Use SEI recovery point to mark keyframe.

Approval Request Comment
[Feature/Bug causing the regression]: 1300296
[User impact if declined]: Streams can't play, or inability to seek in some videos
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: the report includes a link to github but report. can test this one
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: It could potentially leads to allowing seek point that aren't immediately decodable, leading to some short visual artifact (looks like square) but those will always settle themselves quickly. Comparing the behaviour on playing such stream with Safari on mac, and they do that already. So I figured that while rare, it doesn't matter much; additionally it will make seeking much quicker.
[String changes made/needed]:
Attachment #8899533 - Flags: approval-mozilla-beta?
Comment on attachment 8899534 [details]
Bug 1347518 - P2. Don't attempt to determine frame type when encrypted.

Not a new regression but it looks like this fixes issues with some streaming formats. Let's uplift for beta 7.
Attachment #8899534 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8899533 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I reproduced this issue using Fx 55.0a1, build ID: 20170315030215, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 57.0a1, build ID: 20170910220126 and Fx 56.0b10, on Windows 10 x64.

Cheers!
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.