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

[Aurelien Jacobs]

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.

> @@ -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_).

> 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.

> + [...]
> +
> +        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.

> +            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).

Aurel