AVC3 H264 is broken on mac

RESOLVED FIXED in Firefox 43

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla43
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
This is a regression due to bug 1146086.

When using AVC3, we can't immediately initialize the H264 decoder until a SPS has been seen.

However, the H264Wrapper will immediately reject the init promise. This is wrong.

The init promise logic is broken...
(Assignee)

Comment 1

3 years ago
Created attachment 8647948 [details] [diff] [review]
P1. Dont reject init promise when initialising H264Converter.

A decoder isn't created until a SPS and PPS NALs have been detected in the stream. The decoder will be initialised instead lazily later during the input process.
(Assignee)

Updated

3 years ago
Attachment #8647948 - Flags: review?(ayang)
(Assignee)

Comment 2

3 years ago
There is another problem with the H264Converter; the first frame is dropped when the decoder is initialised which causes error when attempting to decode the frame
Assignee: nobody → jyavenard
(Assignee)

Comment 3

3 years ago
Created attachment 8648004 [details] [diff] [review]
P2. Don't drop first sample with SPS/PPS NALs.

It would cause the Apple VT decoder to fail decoding the frame, aborting playback
(Assignee)

Comment 4

3 years ago
Created attachment 8648005 [details] [diff] [review]
P3. Make H264Converter thread safe.

The initialisation promise rather than being launched on the reader's task queue, would be run on a task queue's specific thread
We ended up with a race between two threads owned by the same task queue. Fun.
Attachment #8648005 - Flags: review?(ayang)
(Assignee)

Comment 5

3 years ago
Created attachment 8648006 [details] [diff] [review]
P4. Remove redundant member.
Attachment #8648006 - Flags: review?(ayang)
(Assignee)

Updated

3 years ago
Attachment #8648004 - Flags: review?(ayang)
(Assignee)

Comment 6

3 years ago
Created attachment 8648008 [details] [diff] [review]
P3. Make H264Converter thread safe.

The initialisation promise rather than being launched on the reader's task queue, would be run on a task queue's specific thread
We ended up with a race between two threads owned by the same task queue. Fun.
Attachment #8648008 - Flags: review?(ayang)
(Assignee)

Updated

3 years ago
Attachment #8648005 - Attachment is obsolete: true
Attachment #8648005 - Flags: review?(ayang)
Comment on attachment 8647948 [details] [diff] [review]
P1. Dont reject init promise when initialising H264Converter.

Review of attachment 8647948 [details] [diff] [review]:
-----------------------------------------------------------------

I thought I've tried all the cases, obviously not. Sorry for this regression.
Attachment #8647948 - Flags: review?(ayang) → review+
Attachment #8648004 - Flags: review?(ayang) → review+
Attachment #8648006 - Flags: review?(ayang) → review+
Comment on attachment 8648008 [details] [diff] [review]
P3. Make H264Converter thread safe.

Review of attachment 8648008 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe we don't need to add ReaderTaskQueue() in callback. Task queue can be retrieved from AbstractThread::GetCurrent()->AsTaskQueue(), for example in [1].


[1] https://dxr.mozilla.org/mozilla-central/rev/38c1ea9ccae31700630f1fe0d651e94c0c5b9e1d/dom/media/MediaDecoderStateMachine.cpp#1722
Attachment #8648008 - Flags: review?(ayang) → review+
(Assignee)

Comment 9

3 years ago
Excellent, I had wondered if those existed.

Ideally, I would have liked to use a new taskqueue, but was can't create a taskqueue outside the main thread because there's an assert when it's trying to read a preference which is very silly
https://hg.mozilla.org/mozilla-central/rev/cece40578641
https://hg.mozilla.org/mozilla-central/rev/1d342107d552
https://hg.mozilla.org/mozilla-central/rev/52145910f5b6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.