Closed Bug 1276572 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- wontfix
firefox47 --- affected
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file)

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.
Change made to PushFront() in bug 1190472 was wrong. It will only work if the destination queue was empty
Blocks: 1190472
(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?
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.
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.
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/
Attachment #8757810 - Flags: review?(jwwang)
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 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?
Attachment #8757810 - Flags: review?(jwwang) → review+
Assignee: nobody → jyavenard
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]
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]
https://hg.mozilla.org/mozilla-central/rev/20da5975be4d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.