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

[Aurelien Jacobs]

Anton Khirnov wrote:

> On Sun, May 18, 2008 at 7:01 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
> >
> > 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.
> >
> 
> Ok, but I left it in avformat.h, because I don't know where else to put it.

Ok. It seems there is no avformat global internal header for now, and
avformat.h already contains som ff_ symbols. So it's OK for now.

> We could also let the caller fill all the fields for himself and then
> just pass a pointer to the chapter, but that kind of makes
> av_new_chapter useless, doesn't it?

No very useful indeed. Except that the caller could specify the size
of the AVChapter struct it used (which might be smaller than the
size used by the library if a new field was added in the mean time).

Anyway, this avformat patch looks OK to me. If Michael has no more
objection, I will commit it.

Now a few more things I noticed about the mkv patch:

> Index: libavformat/matroskadec.c
> ===================================================================
> --- libavformat/matroskadec.c	(revision 13199)
> +++ libavformat/matroskadec.c	(working copy)
> @@ -2139,6 +2139,158 @@
>      return res;
>  }
>  
> +static int
> +matroska_parse_chapters(AVFormatContext *s)
> +{
> +    MatroskaDemuxContext *matroska = s->priv_data;
> +    int res = 0;
> +    uint32_t id;
> +
> +    if(s->num_chapters)
> +        ebml_read_skip(matroska);

Hum... You finally added this check at the begin of the function
while keeping it inside the loop. I think the one inside the
loop is enough, and this new one should be removed.

> +                        default:
> +                            av_log(s, AV_LOG_INFO, "Ignoring unknown Chapter atom ID 0x%x\n", id);
> +                        case MATROSKA_ID_CHAPTERUID:
> +                        case MATROSKA_ID_CHAPTERFLAGHIDDEN:
> +                        case EBML_ID_VOID:
> +                            res = ebml_read_skip(matroska);
> +                            break;

> +                            }
> +
> +                            if (matroska->level_up) {
> +                                matroska->level_up--;
> +                                break;
> +                            }
> +                        }
> +

This block seems mis-indented.

> +                    if(!(start == AV_NOPTS_VALUE || end == AV_NOPTS_VALUE))

!(x==a || y==b) can be simplified in (x!=a && y!=b)

Aurel