Closed Bug 1347518 Opened 7 years ago Closed 7 years ago

H264 streams with no IDR fail to play with MSE.

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- verified

People

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

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Bug 1347518 - P1. Use SEI recovery point to mark keyframe.
59 bytes, text/x-review-board-request
jesup
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1347518 - P2. Don't attempt to determine frame type when encrypted.
59 bytes, text/x-review-board-request
cpearce
: review+
lizzard
: approval-mozilla-beta+
Details 
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.
https://drive.google.com/file/d/0B8W7Wb7tGOLuUUJYWm5zcTlPRlE/view?usp=sharing
This link is a fragment of the archive recording of the stream.
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
OS: Unspecified → Windows
Hardware: Unspecified → All
Version: 51 Branch → unspecified
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)
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
Closed: 7 years ago
Flags: needinfo?(mihail.kirillov)
Resolution: --- → INCOMPLETE
Still not working
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
Closed: 7 years ago → 7 years ago
Resolution: --- → WONTFIX
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 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)
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.
Hi James,

Most of our users use Windows. Windows didn't meet this error msg too.
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)
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)
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)
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)
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)
Actually, I don't think this is correct. A SDR with a slice type of 2, is no guarantee that it's a keyframe.
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: jacheng → jyavenard
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Attachment #8890786 - Attachment is obsolete: true
indeed, looking for SEI_RECOVERY_POINT is more meaningful... what a mess.
(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
yes indeed, we need to change hls.js logic and check for SEI recovery instead
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 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)
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.
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 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+
thanks for the reviews.
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
Closed: 7 years ago → 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: MSE
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)
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)
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?
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.

Attachment

General

Creator:
Created:
Updated:
Size: