[FFmpeg-devel] [PATCH] avformat/smacker: Support seeking to first frame

[Andreas Rheinhardt]

Timotej Lazar:
> Add .read_seek function to the smacker demuxer for the special case of
> seeking to ts=0. This is useful because smacker – like bink, with a
> similar implementation – was mostly used to encode clips in video
> games, where random seeks are rare but looping media are common.
> 
> Signed-off-by: Timotej Lazar <timotej.lazar at araneo.si>
> ---
> Unlike the existing bink implementation that always rewinds to start,
> regardless of arguments to seek(), this will return EINVAL when seeking
> to any timestamp other than 0.
> 
> I tried to implement random seeking, but the smacker format is not
> very amenable to that. While it supports keyframes, most videos I
> could find do not use them. Since any video frame can contain an
> incremental palette update, it might be necessary to rewind in any
> case and decode all frames up to the target timestamp.
> 
> This is the first time I dived into ffmpeg, so I’d appreciate any
> feedback or suggestions on how to better approach this. Thanks!
> 
>  libavformat/smacker.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/smacker.c b/libavformat/smacker.c
> index 8b1e185817..19df8edc67 100644
> --- a/libavformat/smacker.c
> +++ b/libavformat/smacker.c
> @@ -230,7 +230,7 @@ static int smacker_read_header(AVFormatContext *s)
>      }
>  
>      smk->curstream = -1;
> -    smk->nextpos = avio_tell(pb);
> +    smk->nextpos = s->internal->data_offset = avio_tell(pb);
>  

This change is unnecessary, as avformat_open_input already sets it to
the exact same value.

>      return 0;
>  }
> @@ -373,6 +373,27 @@ static int smacker_read_close(AVFormatContext *s)
>      return 0;
>  }
>  
> +static int smacker_read_seek(AVFormatContext *s, int stream_index,
> +                             int64_t timestamp, int flags)
> +{
> +    SmackerContext *smk = s->priv_data;
> +
> +    /* only rewinding to start is supported */
> +    if (timestamp != 0) {
> +        av_log(s, AV_LOG_ERROR,
> +               "Random seeks are not supported (can only seek to start).\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +    smk->cur_frame = 0;
> +    smk->curstream = -1;
> +    smk->nextpos = s->internal->data_offset;

You are only telling the demuxer to seek to the beginning and you are
setting the other fields in a way that makes sense if the seek
succeeded. But in a seeking function you should perform the seek
yourself, so that you can check whether the seek was successfull or not.
And you should only reset the other variables in case it was successfull.

Moreover, I have a patchset for the Smacker demuxer [1] and you seem to
know a lot about Smacker, so you might take a look. If you do so, you'll
see that this patchset stops seeking to nextpos every time a new frame
is read (after all, that is the position where already are). But the
patch I'd most like to know your opinion about is patch five [2]. In
particular, I don't know if Smacker supports sparse tracks where the
gaps between points where a track has data is filled by data from other
tracks (think of three audio tracks where track 1 contains data for a
certain language, track 2 contains data for another language and track 3
contains language-independent data (i.e. noise/music when no one is
speaking)). Do you know it? If this is possible, the timestamps
generated will be wrong.

Furthermore, do you have samples with PCM data as audio? I suspect this
to be buggy atm, because it always reads the first four bytes of the
audio packets and treats them as duration (for encoded audio they
contain the size (in bytes) of the decoded audio data which can be used
as duration of the timebase has been set appropriately; yet for PCM
audio this would be redundant and I expect it to not be done (so that
the audio frame size should be used as duration in this case). This is
also what [3] says.

> +    memset(smk->pal, 0, sizeof(smk->pal));
> +    memset(smk->aud_pts, 0, sizeof(smk->aud_pts));
> +
> +    return 0;
> +}
> +
>  AVInputFormat ff_smacker_demuxer = {
>      .name           = "smk",
>      .long_name      = NULL_IF_CONFIG_SMALL("Smacker"),
> @@ -381,4 +402,5 @@ AVInputFormat ff_smacker_demuxer = {
>      .read_header    = smacker_read_header,
>      .read_packet    = smacker_read_packet,
>      .read_close     = smacker_read_close,
> +    .read_seek      = smacker_read_seek,
>  };
> 

- Andreas

[1]: http://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/265033.html
[2]: http://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/265030.html
[3]: https://wiki.multimedia.cx/index.php?title=Smacker#Audio_Track_Chunk