[FFmpeg-devel,v4,3/3] aadec: improve seeking in mp3 content

MP3 frames may not be aligned to aa chunk boundaries. When seeking,
calculate the expected frame offset in the target chunk. Adjust the
timestamp and truncate the next packet accordingly.

This solution works for the majority of tested audio material. For
some rare encodings with mp3 padding or embedded id3 tags, it will
mispredict the correct offset, and at worst skip an extra frame.
---
 libavformat/aadec.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Ping - What about this one? I tested it with about 20 files and it works perfectly
for all of them - except one which has tag/padding. In its case, playback quality
is virtually the same as without the patch, i.e. no harm done.

Cheers, Karsten

> Am 07.07.2018 um 19:41 schrieb Karsten Otto <ottoka@posteo.de>:
> 
> MP3 frames may not be aligned to aa chunk boundaries. When seeking,
> calculate the expected frame offset in the target chunk. Adjust the
> timestamp and truncate the next packet accordingly.
> 
> This solution works for the majority of tested audio material. For
> some rare encodings with mp3 padding or embedded id3 tags, it will
> mispredict the correct offset, and at worst skip an extra frame.
> ---
> libavformat/aadec.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/aadec.c b/libavformat/aadec.c
> index e3c03bc522..2b9e4e526c 100644
> --- a/libavformat/aadec.c
> +++ b/libavformat/aadec.c
> @@ -37,6 +37,7 @@
> #define TEA_BLOCK_SIZE 8
> #define CHAPTER_HEADER_SIZE 8
> #define TIMEPREC 1000
> +#define MP3_FRAME_SIZE 104
> 
> typedef struct AADemuxContext {
>     AVClass *class;
> @@ -50,6 +51,7 @@ typedef struct AADemuxContext {
>     int64_t current_chapter_size;
>     int64_t content_start;
>     int64_t content_end;
> +    int seek_offset;
> } AADemuxContext;
> 
> static int get_second_size(char *codec_name)
> @@ -230,6 +232,7 @@ static int aa_read_header(AVFormatContext *s)
>     ff_update_cur_dts(s, st, 0);
>     avio_seek(pb, start, SEEK_SET);
>     c->current_chapter_size = 0;
> +    c->seek_offset = 0;
> 
>     return 0;
> }
> @@ -295,12 +298,13 @@ static int aa_read_packet(AVFormatContext *s, AVPacket *pkt)
>     if (c->current_chapter_size <= 0)
>         c->current_chapter_size = 0;
> 
> -    ret = av_new_packet(pkt, written);
> +    ret = av_new_packet(pkt, written - c->seek_offset);
>     if (ret < 0)
>         return ret;
> -    memcpy(pkt->data, buf, written);
> +    memcpy(pkt->data, buf + c->seek_offset, written - c->seek_offset);
>     pkt->pos = pos;
> 
> +    c->seek_offset = 0;
>     return 0;
> }
> 
> @@ -344,7 +348,12 @@ static int aa_read_seek(AVFormatContext *s,
>     c->current_chapter_size = chapter_size - chapter_pos;
>     c->chapter_idx = 1 + chapter_idx;
> 
> -    ff_update_cur_dts(s, s->streams[0], ch->start + chapter_pos * TIMEPREC);
> +    // handle extra offset for unaligned frames
> +    if (s->streams[0]->codecpar->codec_id == AV_CODEC_ID_MP3) {
> +        c->seek_offset = (MP3_FRAME_SIZE - chapter_pos % MP3_FRAME_SIZE) % MP3_FRAME_SIZE;
> +    }
> +
> +    ff_update_cur_dts(s, s->streams[0], ch->start + (chapter_pos + c->seek_offset) * TIMEPREC);
> 
>     return 1;
> }
> -- 
> 2.14.3 (Apple Git-98)
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On Sat, Jul 07, 2018 at 07:41:29PM +0200, Karsten Otto wrote:
> MP3 frames may not be aligned to aa chunk boundaries. When seeking,
> calculate the expected frame offset in the target chunk. Adjust the
> timestamp and truncate the next packet accordingly.
> 
> This solution works for the majority of tested audio material. For
> some rare encodings with mp3 padding or embedded id3 tags, it will
> mispredict the correct offset, and at worst skip an extra frame.
> ---
>  libavformat/aadec.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/aadec.c b/libavformat/aadec.c
> index e3c03bc522..2b9e4e526c 100644
> --- a/libavformat/aadec.c
> +++ b/libavformat/aadec.c
> @@ -37,6 +37,7 @@
>  #define TEA_BLOCK_SIZE 8
>  #define CHAPTER_HEADER_SIZE 8
>  #define TIMEPREC 1000
> +#define MP3_FRAME_SIZE 104
>  
>  typedef struct AADemuxContext {
>      AVClass *class;
> @@ -50,6 +51,7 @@ typedef struct AADemuxContext {
>      int64_t current_chapter_size;
>      int64_t content_start;
>      int64_t content_end;
> +    int seek_offset;
>  } AADemuxContext;
>  
>  static int get_second_size(char *codec_name)
> @@ -230,6 +232,7 @@ static int aa_read_header(AVFormatContext *s)
>      ff_update_cur_dts(s, st, 0);
>      avio_seek(pb, start, SEEK_SET);
>      c->current_chapter_size = 0;
> +    c->seek_offset = 0;
>  
>      return 0;
>  }
> @@ -295,12 +298,13 @@ static int aa_read_packet(AVFormatContext *s, AVPacket *pkt)
>      if (c->current_chapter_size <= 0)
>          c->current_chapter_size = 0;
>  
> -    ret = av_new_packet(pkt, written);
> +    ret = av_new_packet(pkt, written - c->seek_offset);
>      if (ret < 0)
>          return ret;
> -    memcpy(pkt->data, buf, written);
> +    memcpy(pkt->data, buf + c->seek_offset, written - c->seek_offset);

what if written < c->seek_offset ? or is this impossible ?

[...]

> Am 11.07.2018 um 20:09 schrieb Michael Niedermayer <michael@niedermayer.cc>:
> 
> Signierter PGP-Teil
> On Sat, Jul 07, 2018 at 07:41:29PM +0200, Karsten Otto wrote:
>> MP3 frames may not be aligned to aa chunk boundaries. When seeking,
>> calculate the expected frame offset in the target chunk. Adjust the
>> timestamp and truncate the next packet accordingly.
>> 
>> This solution works for the majority of tested audio material. For
>> some rare encodings with mp3 padding or embedded id3 tags, it will
>> mispredict the correct offset, and at worst skip an extra frame.
>> ---
>> libavformat/aadec.c | 15 ++++++++++++---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/libavformat/aadec.c b/libavformat/aadec.c
>> index e3c03bc522..2b9e4e526c 100644
>> --- a/libavformat/aadec.c
>> +++ b/libavformat/aadec.c
>> @@ -37,6 +37,7 @@
>> #define TEA_BLOCK_SIZE 8
>> #define CHAPTER_HEADER_SIZE 8
>> #define TIMEPREC 1000
>> +#define MP3_FRAME_SIZE 104
>> 
>> typedef struct AADemuxContext {
>>     AVClass *class;
>> @@ -50,6 +51,7 @@ typedef struct AADemuxContext {
>>     int64_t current_chapter_size;
>>     int64_t content_start;
>>     int64_t content_end;
>> +    int seek_offset;
>> } AADemuxContext;
>> 
>> static int get_second_size(char *codec_name)
>> @@ -230,6 +232,7 @@ static int aa_read_header(AVFormatContext *s)
>>     ff_update_cur_dts(s, st, 0);
>>     avio_seek(pb, start, SEEK_SET);
>>     c->current_chapter_size = 0;
>> +    c->seek_offset = 0;
>> 
>>     return 0;
>> }
>> @@ -295,12 +298,13 @@ static int aa_read_packet(AVFormatContext *s, AVPacket *pkt)
>>     if (c->current_chapter_size <= 0)
>>         c->current_chapter_size = 0;
>> 
>> -    ret = av_new_packet(pkt, written);
>> +    ret = av_new_packet(pkt, written - c->seek_offset);
>>     if (ret < 0)
>>         return ret;
>> -    memcpy(pkt->data, buf, written);
>> +    memcpy(pkt->data, buf + c->seek_offset, written - c->seek_offset);
> 
> what if written < c->seek_offset ? or is this impossible ?
> 
In the normal case this cannot happen, as each chapter is a multiple of the
frame size. They could be equal, which would result in an empty packet.
But I am not quite sure about the worst case I mentioned; to be on the safe
side, I can add an extra check for the last chapter block. Will send a new
patch version.

Cheers, Karsten