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

[Aurelien Jacobs]

On Fri, 16 May 2008 10:19:31 +0200
"Anton Khirnov" <wyskas at gmail.com> wrote:

> On Fri, May 16, 2008 at 1:42 AM, Aurelien Jacobs <aurel at gnuage.org> wrote:
> > Anton Khirnov wrote:
> >
> >> @@ -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.

I agree with this, and I'm not against a public API in general.
I'm just slightly against a public API which makes it slightly
harder to add new fields in AVChapter.
The easiest way to solve this now is to make this function
non-public. This don't prevent adding a public function later.
Another solution is to improve this function so that it can
support inserting new fields in AVChapter without API change.

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

I know about this. But I missed the fact that this check is inside
a while(). So this check is OK here.

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

I absolutely agree that this would be more correct, but that it's
not worth the trouble.

Aurel