1194612 - AVC3 H264 is broken on mac

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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...

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: 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.

Comment 2

[Jean-Yves Avenard [:jya]]

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P2. Don't drop first sample with SPS/PPS NALs.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: 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.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: P4. Remove redundant member.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: 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.

Comment 7

[Alfredo Yang (:alfredo)]

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.

Comment 8

[Alfredo Yang (:alfredo)]

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

Comment 9

[Jean-Yves Avenard [:jya]]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a0ce79b27e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f27d383087
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d2cb05222a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dde632bce46c

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for test bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/37dbde3bfc1b , see https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dde632bce46c

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cece40578641
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d342107d552
https://hg.mozilla.org/integration/mozilla-inbound/rev/52145910f5b6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cece40578641
https://hg.mozilla.org/mozilla-central/rev/1d342107d552
https://hg.mozilla.org/mozilla-central/rev/52145910f5b6