1276572 - when skipping to the next keyframe, first frame returned not always a keyframe

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Found while investigating bug 1274445.

When WebMTrackDemuxer::SkipToNextRandomAccessPoint is called, the next frame demuxed isn't a keyframe.

This cause the ffvp8 decoder to error.

Comment 1

[Jean-Yves Avenard [:jya]]

Change made to PushFront() in bug 1190472 was wrong. It will only work if the destination queue was empty

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #1)
> Change made to PushFront() in bug 1190472 was wrong. It will only work if
> the destination queue was empty

Which patch do you mean?

Comment 3

[Jean-Yves Avenard [:jya]]

Patch 4.

this one:
https://hg.mozilla.org/mozilla-central/rev/0e80a5538a674f3d82aed52d52dd4fbefa074820

it uses Push, so PushFront is actually Push() at the back of the queue.

Comment 4

[Jean-Yves Avenard [:jya]]

This bugs means that more often than not, we will hit a decoding error when skip to the next keyframe is triggered.

I noticed a few times today as I was working on a laptop and had lots of logging active, making my machine particularly slow.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1276572: [webm] Fix MediaRawDataQueue::PushFront. r?jwwang

We want to add MediaRawDataQueue aOther at the front, not at the back.

Review commit: https://reviewboard.mozilla.org/r/56216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56216/

Comment 6

[Jean-Yves Avenard [:jya]]

The problem can occur whenever we are searching for the next keyframe. If the current queue already contained a keyframe plus another frame (not a keyframe), then the keyframe would be added after that 2nd keyframe ; changing the order of the frames.

libvpx was a bit more robust than ffvp8/ffvp9 in handling out of order frame, but ffvpx will just error.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8757810 [details]
MozReview Request: Bug 1276572: [webm] Fix MediaRawDataQueue::PushFront. r?jwwang

https://reviewboard.mozilla.org/r/56216/#review52842

::: dom/media/webm/WebMDemuxer.h:43
(Diff revision 1)
>  
>    void PushFront(already_AddRefed<MediaRawData>&& aItem) {
>      mQueue.push_front(Move(aItem));
>    }
>  
>    void PushFront(MediaRawDataQueue&& aOther) {

Does that guarantee the queue is still sorted after this function rerturns?

Comment 8

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/56216/#review52842

> Does that guarantee the queue is still sorted after this function rerturns?

it only guarantees that aOther is now at the front the queue, and the resulting order is the same as it was before.
e.g. if aOther was [1,2,3,4] and the current queue was [5,6,7,8] after PushFront we have in the queue [1,2,3,4,5,6,7,8]

Comment 9

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/56216/#review52842

> it only guarantees that aOther is now at the front the queue, and the resulting order is the same as it was before.
> e.g. if aOther was [1,2,3,4] and the current queue was [5,6,7,8] after PushFront we have in the queue [1,2,3,4,5,6,7,8]

before we would have had:
if aOther was [1,2,3,4] and the current queue was [5,6,7,8] after PushFront we had in the queue [5,6,7,8,1,2,3,4]

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20da5975be4d

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20da5975be4d