[FFmpeg-devel] [PATCH]support for chapters in mkv container

Anton Khirnov wyskas
Fri May 16 10:19:31 CEST 2008


On Fri, May 16, 2008 at 1:42 AM, Aurelien Jacobs <aurel at gnuage.org> wrote:
> Anton Khirnov wrote:
>
>> On Thu, May 15, 2008 at 3:33 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>
>> >
>> > yes that seems to be the simplest solution.
>> >
>>
>> Done.
>>
>> Index: libavformat/avformat.h
>> ===================================================================
>> --- libavformat/avformat.h    (revision 13155)
>> +++ libavformat/avformat.h    (working copy)
>>
>> [...]
>>
>> +typedef struct AVChapter {
>> +    uint64_t start, end;
>> +    char *title;
>> +} AVChapter;
>
> Every fields of public API structures should have a doxygen comment.
>
>> Index: libavformat/matroskadec.c
>> ===================================================================
>> --- libavformat/matroskadec.c (revision 13155)
>> +++ libavformat/matroskadec.c (working copy)
>> @@ -2133,6 +2133,159 @@
>>      return res;
>>  }
>>
>> +static int
>> +matroska_parse_chapters(AVFormatContext *s)
>> +{
>> +    MatroskaDemuxContext *matroska = s->priv_data;
>> +    int res = 0;
>> +    uint32_t id;
>
>> +    s->num_chapters = 0;
>> +    s->chapters = NULL;
>
> No need to initialize those 2 fields to 0. The whole AVFormatContext
> is set to zero when allocated.
>
>> +            if ((res = ebml_read_master(matroska, &id)) < 0)
>> +                break;
>> +
>> +                while (res == 0) {
>
> Wrong indentation.
>
>> + [...]
>> +
>> +                            case MATROSKA_ID_CHAPTERTIMESTART:
>> +                                res = ebml_read_uint(matroska, &id, &start);
>> +                                break;
>> +
>> +                            case MATROSKA_ID_CHAPTERDISPLAY:
>> +
>
> Useless blank line.
>
>> + [...]
>> +                            if(!title) title = av_strdup("(unnamed)");
>> +                            res = av_new_chapter(s, start * AV_TIME_BASE / 1000000000 , end * AV_TIME_BASE / 1000000000, title);
>
> The av_strdup is not needed. Just use the following inside the
> av_new_chapter() call:
>    title ? title : "(unnamed)"
>
> Also, before calling av_new_chapter(), you could check that start and end
> were read properly (by initializing them to AV_NOPTS_VALUE).
>

Ok.

>
>> @@ -745,6 +753,16 @@
>>  AVProgram *av_new_program(AVFormatContext *s, int id);
>>
>>  /**
>> + * Add a new chapter
>> + *
>> + * @param s media file handle
>> + * @param start chapter start time in AV_TIME_BASE units
>> + * @param end chapter end time in AV_TIME_BASE units
>> + * @param title chapter title
>> + */
>> +int av_new_chapter(AVFormatContext *s, uint64_t start, uint64_t end, char *title);
>
> I'm not sure this is a good idea. Such a public function kind of fixes
> the struct fields in the API.
> IMO, it shouldn't be part of public API (ie. it shouldn't be declared in
> avformat.h and should have a ff_ prefix instead of av_).
>

Well, I don't see why only demuxers should be allowed to add chapters.
For example someone might want to load chapters from a text file.

>
>> + [...]
>> +
>> +        switch (id) {
>> +        case MATROSKA_ID_EDITIONENTRY: {
>> +
>> +            uint64_t end, start;
>> +            char* title = NULL;
>> +            /* if there is more than one chapter edition
>> +             * we take only the first one */
>
>> +            if(s->chapters) {
>> +                    ebml_read_skip(matroska);
>> +                    break;
>> +            }
>
> This check could as well be done at the very begining of the function.
> This would be clearer.
>

The specs say that there could be multiple editions of chapters in a
matroska file, so this check makes sure only the first one is loaded.
Technically the correct way would be to check for EditionFlagDefault
and select that, or make it selectable by user, but i haven't ever
seen an mkv with more than one edition so IMHO it's not worth the
trouble if such files don't exist.

Anton
-------------- next part --------------
A non-text attachment was scrubbed...
Name: chapters_lavf.diff
Type: text/x-diff
Size: 2451 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080516/247a3b36/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: chapters_mkv.diff
Type: text/x-diff
Size: 7509 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080516/247a3b36/attachment-0001.diff>



More information about the ffmpeg-devel mailing list