[FFmpeg-devel] [PATCH]support for chapters in mkv container
Aurelien Jacobs
aurel
Fri May 16 01:42:04 CEST 2008
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
More information about the ffmpeg-devel
mailing list