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

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

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)

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1289438: [ogg] P1. Always seek to the closest keyframe.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1289438: [ogg] P2. Re-enable earlier test now the seeking behaves as expected.

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/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1289438: [ogg] P3 Fix code style.

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

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1289438: [ogg] P4. Use SaferMultDiv where appropriate.

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

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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 ('

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1289438: [ogg] P5. Don't seek back to first buffered position during reset.

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/

Comment 10

[Jean-Yves Avenard [:jya]]

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 11

[Jean-Yves Avenard [:jya]]

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 12

[Jean-Yves Avenard [:jya]]

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 13

[Jean-Yves Avenard [:jya]]

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/

Comment 14

[Brion Vibber]

Comment on attachment 8775034 [details]
Bug 1289438: [ogg] P1. Always seek to the closest keyframe.

https://reviewboard.mozilla.org/r/67380/#review64564

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 17

[Jean-Yves Avenard [:jya]]

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 18

[Jean-Yves Avenard [:jya]]

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 19

[Jean-Yves Avenard [:jya]]

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 20

[Jean-Yves Avenard [:jya]]

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 21

[Jean-Yves Avenard [:jya]]

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 22

[Jean-Yves Avenard [:jya]]

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/

Comment 23

[Jean-Yves Avenard [:jya]]

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 24

[Jean-Yves Avenard [:jya]]

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 25

[Jean-Yves Avenard [:jya]]

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 26

[Jean-Yves Avenard [:jya]]

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 27

[Jean-Yves Avenard [:jya]]

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 28

[Jean-Yves Avenard [:jya]]

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 29

[Jean-Yves Avenard [:jya]]

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 30

[Jean-Yves Avenard [:jya]]

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 31

[Jean-Yves Avenard [:jya]]

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/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
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

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e645059025f
https://hg.mozilla.org/mozilla-central/rev/2f6157213da2
https://hg.mozilla.org/mozilla-central/rev/0f13ecf177c6
https://hg.mozilla.org/mozilla-central/rev/9852c16d65e4
https://hg.mozilla.org/mozilla-central/rev/9fac9a76954f