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

Submitted by Karsten Otto on July 7, 2018, 5:41 p.m.

Details

Message ID 20180707174129.835-4-ottoka@posteo.de
State New
Headers show

Commit Message

Karsten Otto July 7, 2018, 5:41 p.m.
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(-)

Comments

Karsten Otto July 10, 2018, 8:16 p.m.
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
Michael Niedermayer July 11, 2018, 6:09 p.m.
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 ?

[...]
Karsten Otto July 11, 2018, 9:39 p.m.
> 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

Patch hide | download patch | download mbox

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;
 }