diff mbox

[FFmpeg-devel,2/3] lavf/mov: improve `tref/chap` chapter handling

Message ID 20160910020732.24359-2-rodger.combs@gmail.com
State Superseded
Headers show

Commit Message

Rodger Combs Sept. 10, 2016, 2:07 a.m. UTC 
3 parts:
- Supports multiple chapter streams
- Exports regular text chapter streams as opaque data. This prevents consumers
  from showing chapters as if they were regular subtitle streams.
- Exports video chapter streams as thumbnails, and provides the first one as
  an attached_pic.
---
 libavformat/isom.h |  3 ++-
 libavformat/mov.c  | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 47 insertions(+), 8 deletions(-)

Comments

Michael Niedermayer Sept. 11, 2016, 1:41 a.m. UTC | #1 
On Fri, Sep 09, 2016 at 09:07:31PM -0500, Rodger Combs wrote:
> 3 parts:
> - Supports multiple chapter streams
> - Exports regular text chapter streams as opaque data. This prevents consumers
>   from showing chapters as if they were regular subtitle streams.
> - Exports video chapter streams as thumbnails, and provides the first one as
>   an attached_pic.
> ---
>  libavformat/isom.h |  3 ++-
>  libavformat/mov.c  | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index 2246fed..9038057 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -210,7 +210,8 @@ typedef struct MOVContext {
>      unsigned trex_count;
>      int itunes_metadata;  ///< metadata are itunes style
>      int handbrake_version;
> -    int chapter_track;
> +    int *chapter_tracks;
> +    unsigned int nb_chapter_tracks;
>      int use_absolute_path;
>      int ignore_editlist;
>      int ignore_chapters;
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 6e80b93..22ca809 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -3574,7 +3574,18 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  
>  static int mov_read_chap(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
> -    c->chapter_track = avio_rb32(pb);
> +    unsigned i, num;
> +    av_free(c->chapter_tracks);
> +
> +    num = atom.size / 4;
> +    if (!(c->chapter_tracks = av_malloc(num * 4)))

av_malloc_array()

> +        return AVERROR(ENOMEM);

the error return leaves the size and array in an inconsistent tstate


> +
> +    c->nb_chapter_tracks = num;
> +

> +    for (i = 0; i < num; i++)
> +        c->chapter_tracks[i] = avio_rb32(pb);

missing eof check
reading 16gb without checking is not a good idea, it could waste alot
of time

this patch also appears to result in missing codec parameters(pix fmt)
and ffmpeg failing to transcode it
should i upload a file that causes this ?

thx
[...]
Rodger Combs Sept. 13, 2016, 12:25 a.m. UTC | #2 
> On Sep 10, 2016, at 20:41, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> On Fri, Sep 09, 2016 at 09:07:31PM -0500, Rodger Combs wrote:
>> 3 parts:
>> - Supports multiple chapter streams
>> - Exports regular text chapter streams as opaque data. This prevents consumers
>>  from showing chapters as if they were regular subtitle streams.
>> - Exports video chapter streams as thumbnails, and provides the first one as
>>  an attached_pic.
>> ---
>> libavformat/isom.h |  3 ++-
>> libavformat/mov.c  | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 47 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index 2246fed..9038057 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -210,7 +210,8 @@ typedef struct MOVContext {
>>     unsigned trex_count;
>>     int itunes_metadata;  ///< metadata are itunes style
>>     int handbrake_version;
>> -    int chapter_track;
>> +    int *chapter_tracks;
>> +    unsigned int nb_chapter_tracks;
>>     int use_absolute_path;
>>     int ignore_editlist;
>>     int ignore_chapters;
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 6e80b93..22ca809 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -3574,7 +3574,18 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> 
>> static int mov_read_chap(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> {
>> -    c->chapter_track = avio_rb32(pb);
>> +    unsigned i, num;
>> +    av_free(c->chapter_tracks);
>> +
>> +    num = atom.size / 4;
>> +    if (!(c->chapter_tracks = av_malloc(num * 4)))
> 
> av_malloc_array()

Done

> 
>> +        return AVERROR(ENOMEM);
> 
> the error return leaves the size and array in an inconsistent tstate

Fixed

> 
> 
>> +
>> +    c->nb_chapter_tracks = num;
>> +
> 
>> +    for (i = 0; i < num; i++)
>> +        c->chapter_tracks[i] = avio_rb32(pb);
> 
> missing eof check
> reading 16gb without checking is not a good idea, it could waste alot
> of time

Added

> 
> this patch also appears to result in missing codec parameters(pix fmt)
> and ffmpeg failing to transcode it
> should i upload a file that causes this ?

Would appreciate one, yeah

> 
> thx
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Republics decline into democracies and democracies degenerate into
> despotisms. -- Aristotle
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 2246fed..9038057 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -210,7 +210,8 @@  typedef struct MOVContext {
     unsigned trex_count;
     int itunes_metadata;  ///< metadata are itunes style
     int handbrake_version;
-    int chapter_track;
+    int *chapter_tracks;
+    unsigned int nb_chapter_tracks;
     int use_absolute_path;
     int ignore_editlist;
     int ignore_chapters;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 6e80b93..22ca809 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -3574,7 +3574,18 @@  static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
 static int mov_read_chap(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-    c->chapter_track = avio_rb32(pb);
+    unsigned i, num;
+    av_free(c->chapter_tracks);
+
+    num = atom.size / 4;
+    if (!(c->chapter_tracks = av_malloc(num * 4)))
+        return AVERROR(ENOMEM);
+
+    c->nb_chapter_tracks = num;
+
+    for (i = 0; i < num; i++)
+        c->chapter_tracks[i] = avio_rb32(pb);
+
     return 0;
 }
 
@@ -4637,25 +4648,50 @@  static int mov_probe(AVProbeData *p)
 static void mov_read_chapters(AVFormatContext *s)
 {
     MOVContext *mov = s->priv_data;
-    AVStream *st = NULL;
+    AVStream *st;
     MOVStreamContext *sc;
     int64_t cur_pos;
-    int i;
+    int i, j;
+    int chapter_track;
 
+    for (j = 0; j < mov->nb_chapter_tracks; j++) {
+    chapter_track = mov->chapter_tracks[j];
+    st = NULL;
     for (i = 0; i < s->nb_streams; i++)
-        if (s->streams[i]->id == mov->chapter_track) {
+        if (s->streams[i]->id == chapter_track) {
             st = s->streams[i];
             break;
         }
     if (!st) {
         av_log(s, AV_LOG_ERROR, "Referenced QT chapter track not found\n");
-        return;
+        continue;
     }
 
-    st->discard = AVDISCARD_ALL;
     sc = st->priv_data;
     cur_pos = avio_tell(sc->pb);
 
+    if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
+        st->disposition |= AV_DISPOSITION_ATTACHED_PIC | AV_DISPOSITION_TIMED_THUMBNAILS;
+        if (st->nb_index_entries) {
+            // Retrieve the first frame, if possible
+            AVPacket pkt;
+            AVIndexEntry *sample = &st->index_entries[0];
+            if (avio_seek(sc->pb, sample->pos, SEEK_SET) != sample->pos) {
+                av_log(s, AV_LOG_ERROR, "Failed to retrieve first frame\n");
+                goto finish;
+            }
+
+            if (av_get_packet(sc->pb, &pkt, sample->size) < 0)
+                goto finish;
+
+            st->attached_pic              = pkt;
+            st->attached_pic.stream_index = st->index;
+            st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
+        }
+    } else {
+    st->codec->codec_type = AVMEDIA_TYPE_DATA;
+    st->codec->codec_id = AV_CODEC_ID_BIN_DATA;
+    st->discard = AVDISCARD_ALL;
     for (i = 0; i < st->nb_index_entries; i++) {
         AVIndexEntry *sample = &st->index_entries[i];
         int64_t end = i+1 < st->nb_index_entries ? st->index_entries[i+1].timestamp : st->duration;
@@ -4704,8 +4740,10 @@  static void mov_read_chapters(AVFormatContext *s)
         avpriv_new_chapter(s, i, st->time_base, sample->timestamp, end, title);
         av_freep(&title);
     }
+    }
 finish:
     avio_seek(sc->pb, cur_pos, SEEK_SET);
+    }
 }
 
 static int parse_timecode_in_framenum_format(AVFormatContext *s, AVStream *st,
@@ -5028,7 +5066,7 @@  static int mov_read_header(AVFormatContext *s)
     av_log(mov->fc, AV_LOG_TRACE, "on_parse_exit_offset=%"PRId64"\n", avio_tell(pb));
 
     if (pb->seekable) {
-        if (mov->chapter_track > 0 && !mov->ignore_chapters)
+        if (mov->nb_chapter_tracks > 0 && !mov->ignore_chapters)
             mov_read_chapters(s);
         for (i = 0; i < s->nb_streams; i++)
             if (s->streams[i]->codecpar->codec_tag == AV_RL32("tmcd")) {