diff mbox series

[FFmpeg-devel] avformat/wavdec: add support for chapters

Message ID 20200831122011.2786-1-onemda@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] avformat/wavdec: add support for chapters
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Paul B Mahol Aug. 31, 2020, 12:20 p.m. UTC
Support parsing 'cue ' and 'adtl' chunks.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/wavdec.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Anton Khirnov Sept. 1, 2020, 11:43 a.m. UTC | #1
Quoting Paul B Mahol (2020-08-31 14:20:11)
> Support parsing 'cue ' and 'adtl' chunks.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/wavdec.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)

Looks ok beyond some nits.

> 
> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
> index 5b3c481421..545f04c742 100644
> --- a/libavformat/wavdec.c
> +++ b/libavformat/wavdec.c
> @@ -500,6 +500,7 @@ static int wav_read_header(AVFormatContext *s)
>              wav->smv_cur_pt = 0;
>              goto break_loop;
>          case MKTAG('L', 'I', 'S', 'T'):
> +        case MKTAG('l', 'i', 's', 't'):
>              if (size < 4) {
>                  av_log(s, AV_LOG_ERROR, "too short LIST tag\n");
>                  return AVERROR_INVALIDDATA;
> @@ -507,6 +508,37 @@ static int wav_read_header(AVFormatContext *s)
>              switch (avio_rl32(pb)) {
>              case MKTAG('I', 'N', 'F', 'O'):
>                  ff_read_riff_info(s, size - 4);
> +                break;
> +            case MKTAG('a', 'd', 't', 'l'):
> +                if (s->nb_chapters > 0) {
> +                    while (avio_tell(pb) < next_tag_ofs) {
> +                        AVChapter *chapter = NULL;
> +                        char cue_label[512];
> +                        unsigned id, sub_size;
> +
> +                        if (avio_feof(pb))
> +                            break;

nit: would look better in the loop condition

> +                        if (avio_rl32(pb) != MKTAG('l', 'a', 'b', 'l'))
> +                            break;
> +
> +                        sub_size = avio_rl32(pb);
> +                        if (sub_size < 5)
> +                            break;
> +                        id       = avio_rl32(pb);
> +                        avio_get_str(pb, sub_size - 4, cue_label, sizeof(cue_label));
> +                        avio_skip(pb, avio_tell(pb) & 1);
> +
> +                        for (int i = 0; i < s->nb_chapters; i++) {
> +                            if (s->chapters[i]->id == id) {
> +                                chapter = s->chapters[i];
> +                                break;
> +                            }
> +                        }
> +                        if (chapter)
> +                            av_dict_set(&chapter->metadata, "title", cue_label, 0);

nit: could be merged into the loop above
Anton Khirnov Sept. 1, 2020, 11:43 a.m. UTC | #2
Quoting Paul B Mahol (2020-08-31 14:20:11)
> Support parsing 'cue ' and 'adtl' chunks.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/wavdec.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)

Also, FATE tests would be nice.
Paul B Mahol Sept. 1, 2020, 12:23 p.m. UTC | #3
On 9/1/20, Anton Khirnov <anton@khirnov.net> wrote:
> Quoting Paul B Mahol (2020-08-31 14:20:11)
>> Support parsing 'cue ' and 'adtl' chunks.
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavformat/wavdec.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>
> Looks ok beyond some nits.
>
>>
>> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
>> index 5b3c481421..545f04c742 100644
>> --- a/libavformat/wavdec.c
>> +++ b/libavformat/wavdec.c
>> @@ -500,6 +500,7 @@ static int wav_read_header(AVFormatContext *s)
>>              wav->smv_cur_pt = 0;
>>              goto break_loop;
>>          case MKTAG('L', 'I', 'S', 'T'):
>> +        case MKTAG('l', 'i', 's', 't'):
>>              if (size < 4) {
>>                  av_log(s, AV_LOG_ERROR, "too short LIST tag\n");
>>                  return AVERROR_INVALIDDATA;
>> @@ -507,6 +508,37 @@ static int wav_read_header(AVFormatContext *s)
>>              switch (avio_rl32(pb)) {
>>              case MKTAG('I', 'N', 'F', 'O'):
>>                  ff_read_riff_info(s, size - 4);
>> +                break;
>> +            case MKTAG('a', 'd', 't', 'l'):
>> +                if (s->nb_chapters > 0) {
>> +                    while (avio_tell(pb) < next_tag_ofs) {
>> +                        AVChapter *chapter = NULL;
>> +                        char cue_label[512];
>> +                        unsigned id, sub_size;
>> +
>> +                        if (avio_feof(pb))
>> +                            break;
>
> nit: would look better in the loop condition

Done.

>
>> +                        if (avio_rl32(pb) != MKTAG('l', 'a', 'b', 'l'))
>> +                            break;
>> +
>> +                        sub_size = avio_rl32(pb);
>> +                        if (sub_size < 5)
>> +                            break;
>> +                        id       = avio_rl32(pb);
>> +                        avio_get_str(pb, sub_size - 4, cue_label,
>> sizeof(cue_label));
>> +                        avio_skip(pb, avio_tell(pb) & 1);
>> +
>> +                        for (int i = 0; i < s->nb_chapters; i++) {
>> +                            if (s->chapters[i]->id == id) {
>> +                                chapter = s->chapters[i];
>> +                                break;
>> +                            }
>> +                        }
>> +                        if (chapter)
>> +                            av_dict_set(&chapter->metadata, "title",
>> cue_label, 0);
>
> nit: could be merged into the loop above

Done.

Also fixed size check to not ignore 4 read bytes, and applied.

Thanks.

>
> --
> Anton Khirnov
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
index 5b3c481421..545f04c742 100644
--- a/libavformat/wavdec.c
+++ b/libavformat/wavdec.c
@@ -500,6 +500,7 @@  static int wav_read_header(AVFormatContext *s)
             wav->smv_cur_pt = 0;
             goto break_loop;
         case MKTAG('L', 'I', 'S', 'T'):
+        case MKTAG('l', 'i', 's', 't'):
             if (size < 4) {
                 av_log(s, AV_LOG_ERROR, "too short LIST tag\n");
                 return AVERROR_INVALIDDATA;
@@ -507,6 +508,37 @@  static int wav_read_header(AVFormatContext *s)
             switch (avio_rl32(pb)) {
             case MKTAG('I', 'N', 'F', 'O'):
                 ff_read_riff_info(s, size - 4);
+                break;
+            case MKTAG('a', 'd', 't', 'l'):
+                if (s->nb_chapters > 0) {
+                    while (avio_tell(pb) < next_tag_ofs) {
+                        AVChapter *chapter = NULL;
+                        char cue_label[512];
+                        unsigned id, sub_size;
+
+                        if (avio_feof(pb))
+                            break;
+                        if (avio_rl32(pb) != MKTAG('l', 'a', 'b', 'l'))
+                            break;
+
+                        sub_size = avio_rl32(pb);
+                        if (sub_size < 5)
+                            break;
+                        id       = avio_rl32(pb);
+                        avio_get_str(pb, sub_size - 4, cue_label, sizeof(cue_label));
+                        avio_skip(pb, avio_tell(pb) & 1);
+
+                        for (int i = 0; i < s->nb_chapters; i++) {
+                            if (s->chapters[i]->id == id) {
+                                chapter = s->chapters[i];
+                                break;
+                            }
+                        }
+                        if (chapter)
+                            av_dict_set(&chapter->metadata, "title", cue_label, 0);
+                    }
+                }
+                break;
             }
             break;
         case MKTAG('I', 'D', '3', ' '):
@@ -521,6 +553,24 @@  static int wav_read_header(AVFormatContext *s)
             ff_id3v2_free_extra_meta(&id3v2_extra_meta);
             }
             break;
+        case MKTAG('c', 'u', 'e', ' '):
+            if (size >= 4 && got_fmt && st->codecpar->sample_rate > 0) {
+                AVRational tb = {1, st->codecpar->sample_rate};
+                unsigned nb_cues = avio_rl32(pb);
+
+                if (size >= nb_cues * 24LL) {
+                    for (int i = 0; i < nb_cues; i++) {
+                        unsigned offset, id = avio_rl32(pb);
+
+                        avio_skip(pb, 16);
+                        offset = avio_rl32(pb);
+
+                        if (!avpriv_new_chapter(s, id, tb, offset, AV_NOPTS_VALUE, NULL))
+                            return AVERROR(ENOMEM);
+                    }
+                }
+            }
+            break;
         }
 
         /* seek to next tag unless we know that we'll run into EOF */