Closed Bug 1289438 Opened 3 years ago Closed 3 years ago

Seeking into indexed ogg doesn't return last keyframe prior target time

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(5 files)

When seeking into an ogg file with a page index ; the seek operation stops on the first keyframe it finds.

The page index only provides a hash and doesn't necessarily provide a link to the page that is being searched for.

An example is in the file bug516323.indexed.ogv

the mochitest dom/media/test_fastseek.html attempts to seek to a keyframe that is located at time 0.46 (btw it's the wrong time as the video keyframe is found at 0.583333).
But even attempting to seek at say 1.7s, the index is made of only two entries. The index containing the target time is the first entry which has a start time of 0.

And as such, the first video frame returned is the first frame of the first page.

You can't actually seek to the nearest keyframe.

It would be required to start demuxing from that first page until we find the packet with the closest keyframe.
The issue could be that there are no other keyframe in which case we performed the search unnecessarily.
We may want to limit the search to say 10s ahead max (similar to what we done with webm) so that we don't end up demuxing the whole file and keeping all the demuxed packets in ram.
the issue really only matters for fast seek, we may not care about it (though for accurate seek, we would also unnecessarily decode many frames that don't have to be decoded)
Note that the closest keyframes are for video 0.666667 and audio 0.645805, however the current OggReader still incorrectly seeks audio to an
earlier time as it seek to the index page boundary.

Review commit: https://reviewboard.mozilla.org/r/67382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67382/
Attachment #8775034 - Flags: review?(brion)
Assignee: nobody → jyavenard
Comment on attachment 8775035 [details]
Bug 1289438: [ogg] P2. Re-enable earlier test now the seeking behaves as expected.

https://reviewboard.mozilla.org/r/67382/#review64444
Attachment #8775035 - Flags: review+
Comment on attachment 8775036 [details]
Bug 1289438: [ogg] P3 Fix code style.

https://reviewboard.mozilla.org/r/67384/#review64446

r+ with nits:

::: dom/media/ogg/OggCodecState.h:45
(Diff revision 1)
>  namespace mozilla {
>  
>  // Deallocates a packet, used in OggPacketQueue below.
> -class OggPacketDeallocator : public nsDequeFunctor {
> +class OggPacketDeallocator : public nsDequeFunctor
> +{
>    virtual void* operator() (void* aPacket) {

Style: Non-empty method definition's '{' on separate line too.

::: dom/media/ogg/OggCodecState.h:84
(Diff revision 1)
> -class OggCodecState {
> +class OggCodecState
> +{
>  public:
>    typedef mozilla::MetadataTags MetadataTags;
>    // Ogg types we know about
>    enum CodecType {

Style: enum's '{' on separate line? Note: It's not actually mentioned in the coding style, so I'm going for consistency with structs; but a lot of code is like this, so I wouldn't worry either way, up to you.

::: dom/media/ogg/OggCodecState.cpp:1019
(Diff revision 1)
>    nsAutoRef<ogg_packet> autoRelease(aPacket);
>    switch(mPacketCount++) {
>      // Parse the id header.
> -    case 0: {
> +    case 0:
> -        mParser = new OpusParser;
> +      mParser = new OpusParser;
> -        if(!mParser->DecodeHeader(aPacket->packet, aPacket->bytes)) {
> +      if(!mParser->DecodeHeader(aPacket->packet, aPacket->bytes)) {

'if(' -> 'if ('

::: dom/media/ogg/OggCodecState.cpp:1034
(Diff revision 1)
> -    }
> -    break;
> +      break;
>  
>      // Parse the metadata header.
> -    case 1: {
> +    case 1:
> -        if(!mParser->DecodeTags(aPacket->packet, aPacket->bytes)) {
> +      if(!mParser->DecodeTags(aPacket->packet, aPacket->bytes)) {

'if(' -> 'if ('
Attachment #8775036 - Flags: review?(gsquelart) → review+
Comment on attachment 8775037 [details]
Bug 1289438: [ogg] P4. Use SaferMultDiv where appropriate.

https://reviewboard.mozilla.org/r/67386/#review64448

r+, but in the commit description: 'SafeMultDiv' -> 'SaferMultDiv' (add missing 'r').
Attachment #8775037 - Flags: review?(gsquelart) → review+
A call to reset is always followed by a call to Seek; seeking is an heavy operation with ogg so let's minimize the number of times we are actually seeking.

Review commit: https://reviewboard.mozilla.org/r/67414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67414/
Attachment #8775131 - Flags: review?(gsquelart)
Attachment #8775035 - Flags: review+
Comment on attachment 8775034 [details]
Bug 1289438: [ogg] P1. Always seek to the closest keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67380/diff/1-2/
Comment on attachment 8775035 [details]
Bug 1289438: [ogg] P2. Re-enable earlier test now the seeking behaves as expected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67382/diff/1-2/
Comment on attachment 8775036 [details]
Bug 1289438: [ogg] P3 Fix code style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67384/diff/1-2/
Comment on attachment 8775037 [details]
Bug 1289438: [ogg] P4. Use SaferMultDiv where appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67386/diff/1-2/
Attachment #8775034 - Flags: review?(brion) → review+
Comment on attachment 8775034 [details]
Bug 1289438: [ogg] P1. Always seek to the closest keyframe.

https://reviewboard.mozilla.org/r/67380/#review64564
Comment on attachment 8775035 [details]
Bug 1289438: [ogg] P2. Re-enable earlier test now the seeking behaves as expected.

https://reviewboard.mozilla.org/r/67382/#review64610
Attachment #8775035 - Flags: review?(cpearce) → review+
Comment on attachment 8775131 [details]
Bug 1289438: [ogg] P5. Don't seek back to first buffered position during reset.

https://reviewboard.mozilla.org/r/67414/#review64636
Attachment #8775131 - Flags: review?(gsquelart) → review+
Comment on attachment 8775034 [details]
Bug 1289438: [ogg] P1. Always seek to the closest keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67380/diff/2-3/
Comment on attachment 8775035 [details]
Bug 1289438: [ogg] P2. Re-enable earlier test now the seeking behaves as expected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67382/diff/2-3/
Comment on attachment 8775036 [details]
Bug 1289438: [ogg] P3 Fix code style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67384/diff/2-3/
Comment on attachment 8775037 [details]
Bug 1289438: [ogg] P4. Use SaferMultDiv where appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67386/diff/2-3/
Comment on attachment 8775131 [details]
Bug 1289438: [ogg] P5. Don't seek back to first buffered position during reset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67414/diff/1-2/
Comment on attachment 8775034 [details]
Bug 1289438: [ogg] P1. Always seek to the closest keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67380/diff/3-4/
Attachment #8775037 - Attachment description: Bug 1289438: [ogg] P4. Use SafeMultDiv where appropriate. → Bug 1289438: [ogg] P4. Use SaferMultDiv where appropriate.
Comment on attachment 8775035 [details]
Bug 1289438: [ogg] P2. Re-enable earlier test now the seeking behaves as expected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67382/diff/3-4/
Comment on attachment 8775036 [details]
Bug 1289438: [ogg] P3 Fix code style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67384/diff/3-4/
Comment on attachment 8775037 [details]
Bug 1289438: [ogg] P4. Use SaferMultDiv where appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67386/diff/3-4/
Comment on attachment 8775131 [details]
Bug 1289438: [ogg] P5. Don't seek back to first buffered position during reset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67414/diff/2-3/
Comment on attachment 8775034 [details]
Bug 1289438: [ogg] P1. Always seek to the closest keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67380/diff/4-5/
Comment on attachment 8775035 [details]
Bug 1289438: [ogg] P2. Re-enable earlier test now the seeking behaves as expected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67382/diff/4-5/
Comment on attachment 8775036 [details]
Bug 1289438: [ogg] P3 Fix code style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67384/diff/4-5/
Comment on attachment 8775037 [details]
Bug 1289438: [ogg] P4. Use SaferMultDiv where appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67386/diff/4-5/
Comment on attachment 8775131 [details]
Bug 1289438: [ogg] P5. Don't seek back to first buffered position during reset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67414/diff/3-4/
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e645059025f
[ogg] P1. Always seek to the closest keyframe. r=brion+1012
https://hg.mozilla.org/integration/autoland/rev/2f6157213da2
[ogg] P2. Re-enable earlier test now the seeking behaves as expected. r=cpearce,gerald
https://hg.mozilla.org/integration/autoland/rev/0f13ecf177c6
[ogg] P3 Fix code style. r=gerald
https://hg.mozilla.org/integration/autoland/rev/9852c16d65e4
[ogg] P4. Use SaferMultDiv where appropriate. r=gerald
https://hg.mozilla.org/integration/autoland/rev/9fac9a76954f
[ogg] P5. Don't seek back to first buffered position during reset. r=gerald
You need to log in before you can comment on or make changes to this bug.