diff mbox

[FFmpeg-devel] avformat/id3v2enc: write CTOC too

Message ID 20190604144538.18166-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol June 4, 2019, 2:45 p.m. UTC 
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/id3v2enc.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

James Almer June 4, 2019, 4:02 p.m. UTC | #1 
On 6/4/2019 11:45 AM, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/id3v2enc.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/libavformat/id3v2enc.c b/libavformat/id3v2enc.c
> index ffe358f019..9b72614447 100644
> --- a/libavformat/id3v2enc.c
> +++ b/libavformat/id3v2enc.c
> @@ -255,6 +255,39 @@ static int write_metadata(AVIOContext *pb, AVDictionary **metadata,
>      return 0;
>  }
>  
> +static int write_ctoc(AVFormatContext *s, ID3v2EncContext *id3, int enc)
> +{
> +    uint8_t *dyn_buf = NULL;
> +    AVIOContext *dyn_bc = NULL;
> +    char name[123];
> +    int len, ret;
> +
> +    if ((ret = avio_open_dyn_buf(&dyn_bc)) < 0)
> +        goto fail;
> +
> +    id3->len += avio_put_str(dyn_bc, "toc");
> +    avio_wb16(dyn_bc, 0x03);
> +    avio_w8(dyn_bc, s->nb_chapters);
> +    for (int i = 0; i < s->nb_chapters; i++) {
> +        snprintf(name, 122, "ch%d", i);
> +        id3->len += avio_put_str(dyn_bc, name);
> +    }
> +    len = avio_close_dyn_buf(dyn_bc, &dyn_buf);
> +    id3->len += 16 + ID3v2_HEADER_SIZE;
> +
> +    avio_wb32(s->pb, MKBETAG('C', 'T', 'O', 'C'));
> +    avio_wb32(s->pb, len);
> +    avio_wb16(s->pb, 0);
> +    avio_write(s->pb, dyn_buf, len);
> +
> +fail:
> +    if (dyn_bc && !dyn_buf)
> +        avio_close_dyn_buf(dyn_bc, &dyn_buf);
> +    av_freep(&dyn_buf);
> +
> +    return ret;
> +}
> +
>  static int write_chapter(AVFormatContext *s, ID3v2EncContext *id3, int id, int enc)
>  {
>      const AVRational time_base = {1, 1000};
> @@ -306,6 +339,9 @@ int ff_id3v2_write_metadata(AVFormatContext *s, ID3v2EncContext *id3)
>      if ((ret = write_metadata(s->pb, &s->metadata, id3, enc)) < 0)
>          return ret;
>  
> +    if ((ret = write_ctoc(s, id3, enc)) < 0)

Shouldn't you check that s->nb_chapters is > 0 before calling this? Or
within that function, alternatively.

Even if CTOC could be written with 0 chapters, it would be an
unnecessary bloat in the output file.

> +        return ret;
> +
>      for (i = 0; i < s->nb_chapters; i++) {
>          if ((ret = write_chapter(s, id3, i, enc)) < 0)
>              return ret;
>
Paul B Mahol June 4, 2019, 8:26 p.m. UTC | #2 
On 6/4/19, James Almer <jamrial@gmail.com> wrote:
> On 6/4/2019 11:45 AM, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavformat/id3v2enc.c | 36 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/libavformat/id3v2enc.c b/libavformat/id3v2enc.c
>> index ffe358f019..9b72614447 100644
>> --- a/libavformat/id3v2enc.c
>> +++ b/libavformat/id3v2enc.c
>> @@ -255,6 +255,39 @@ static int write_metadata(AVIOContext *pb,
>> AVDictionary **metadata,
>>      return 0;
>>  }
>>
>> +static int write_ctoc(AVFormatContext *s, ID3v2EncContext *id3, int enc)
>> +{
>> +    uint8_t *dyn_buf = NULL;
>> +    AVIOContext *dyn_bc = NULL;
>> +    char name[123];
>> +    int len, ret;
>> +
>> +    if ((ret = avio_open_dyn_buf(&dyn_bc)) < 0)
>> +        goto fail;
>> +
>> +    id3->len += avio_put_str(dyn_bc, "toc");
>> +    avio_wb16(dyn_bc, 0x03);
>> +    avio_w8(dyn_bc, s->nb_chapters);
>> +    for (int i = 0; i < s->nb_chapters; i++) {
>> +        snprintf(name, 122, "ch%d", i);
>> +        id3->len += avio_put_str(dyn_bc, name);
>> +    }
>> +    len = avio_close_dyn_buf(dyn_bc, &dyn_buf);
>> +    id3->len += 16 + ID3v2_HEADER_SIZE;
>> +
>> +    avio_wb32(s->pb, MKBETAG('C', 'T', 'O', 'C'));
>> +    avio_wb32(s->pb, len);
>> +    avio_wb16(s->pb, 0);
>> +    avio_write(s->pb, dyn_buf, len);
>> +
>> +fail:
>> +    if (dyn_bc && !dyn_buf)
>> +        avio_close_dyn_buf(dyn_bc, &dyn_buf);
>> +    av_freep(&dyn_buf);
>> +
>> +    return ret;
>> +}
>> +
>>  static int write_chapter(AVFormatContext *s, ID3v2EncContext *id3, int
>> id, int enc)
>>  {
>>      const AVRational time_base = {1, 1000};
>> @@ -306,6 +339,9 @@ int ff_id3v2_write_metadata(AVFormatContext *s,
>> ID3v2EncContext *id3)
>>      if ((ret = write_metadata(s->pb, &s->metadata, id3, enc)) < 0)
>>          return ret;
>>
>> +    if ((ret = write_ctoc(s, id3, enc)) < 0)
>
> Shouldn't you check that s->nb_chapters is > 0 before calling this? Or
> within that function, alternatively.
>
> Even if CTOC could be written with 0 chapters, it would be an
> unnecessary bloat in the output file.
>

Agreed, will add check.

>> +        return ret;
>> +
>>      for (i = 0; i < s->nb_chapters; i++) {
>>          if ((ret = write_chapter(s, id3, i, enc)) < 0)
>>              return ret;
>>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer June 5, 2019, 9:30 p.m. UTC | #3 
On Tue, Jun 04, 2019 at 04:45:38PM +0200, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/id3v2enc.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)

if this is written in a fate test then it will need an update
(this patch as it is ATM does write it in fate-lavf-fate-mp3)

[...]
Paul B Mahol June 6, 2019, 7:08 a.m. UTC | #4 
On 6/5/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Tue, Jun 04, 2019 at 04:45:38PM +0200, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavformat/id3v2enc.c | 36 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>
> if this is written in a fate test then it will need an update
> (this patch as it is ATM does write it in fate-lavf-fate-mp3)
>

What are you talking about? No tests need update for fate.

> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> While the State exists there can be no freedom; when there is freedom there
> will be no State. -- Vladimir Lenin
>
diff mbox

Patch

diff --git a/libavformat/id3v2enc.c b/libavformat/id3v2enc.c
index ffe358f019..9b72614447 100644
--- a/libavformat/id3v2enc.c
+++ b/libavformat/id3v2enc.c
@@ -255,6 +255,39 @@  static int write_metadata(AVIOContext *pb, AVDictionary **metadata,
     return 0;
 }
 
+static int write_ctoc(AVFormatContext *s, ID3v2EncContext *id3, int enc)
+{
+    uint8_t *dyn_buf = NULL;
+    AVIOContext *dyn_bc = NULL;
+    char name[123];
+    int len, ret;
+
+    if ((ret = avio_open_dyn_buf(&dyn_bc)) < 0)
+        goto fail;
+
+    id3->len += avio_put_str(dyn_bc, "toc");
+    avio_wb16(dyn_bc, 0x03);
+    avio_w8(dyn_bc, s->nb_chapters);
+    for (int i = 0; i < s->nb_chapters; i++) {
+        snprintf(name, 122, "ch%d", i);
+        id3->len += avio_put_str(dyn_bc, name);
+    }
+    len = avio_close_dyn_buf(dyn_bc, &dyn_buf);
+    id3->len += 16 + ID3v2_HEADER_SIZE;
+
+    avio_wb32(s->pb, MKBETAG('C', 'T', 'O', 'C'));
+    avio_wb32(s->pb, len);
+    avio_wb16(s->pb, 0);
+    avio_write(s->pb, dyn_buf, len);
+
+fail:
+    if (dyn_bc && !dyn_buf)
+        avio_close_dyn_buf(dyn_bc, &dyn_buf);
+    av_freep(&dyn_buf);
+
+    return ret;
+}
+
 static int write_chapter(AVFormatContext *s, ID3v2EncContext *id3, int id, int enc)
 {
     const AVRational time_base = {1, 1000};
@@ -306,6 +339,9 @@  int ff_id3v2_write_metadata(AVFormatContext *s, ID3v2EncContext *id3)
     if ((ret = write_metadata(s->pb, &s->metadata, id3, enc)) < 0)
         return ret;
 
+    if ((ret = write_ctoc(s, id3, enc)) < 0)
+        return ret;
+
     for (i = 0; i < s->nb_chapters; i++) {
         if ((ret = write_chapter(s, id3, i, enc)) < 0)
             return ret;