[MPlayer-dev-eng] [PATCH] Chapter support improvement

[Alban Bedel]

On Mon, 10 Dec 2007 18:59:28 +0800
Ulion <ulion2002 at gmail.com> wrote:

> Hello,
> 
> I recent found chapter in mkv is really a good design, certainly dvd
> also support it. But for mplayer, its chapter support is not good
> enough, then I wrote several patches to improve it.
> 
> First patch adds 3 functions for demuxer to get chapter infos:
> 
> +/// Get current chapter index if available.
> +int demuxer_get_current_chapter(demuxer_t *demuxer);
> +/// Get chapter name by index.
> +char *demuxer_chapter_name(demuxer_t *demuxer, int chapter);
> +/// Get total chapter number.
> +int demuxer_chapter_count(demuxer_t *demuxer);
> 
> Second patch make chapter become a mplayer property, now we can
> get/set/print/step up/step down on the chapter property.
> 
> Third patch add chapter support for libmenu, it bases on Benjanmin's
> menu_chapsel.c patch, but using functions from my first patch.

That look good :) Just a few comments:

> +chapter [N]
> +    Seek to specific chapter. Here [N] is an integer.
> +    If [N] is 0, will seek forward to next chapter (default).
> +    If [N] larger than 0, seek to the [N]th chapter.

Is this really usefull? There is already the seek_chapter command.
If you just want an alias then use the same cmd id (and parameters
evidently).

> +        chapter = demuxer_get_current_chapter(mpctx->demuxer);
> +        if (chapter < 0)
> +            snprintf(*(char **) arg, 63, "(%d) %s", chapter+1,
> +                     MSGTR_Unknown);

It would make more sense to check that right at the start and return
UNAVAILABLE if there is no chapter.

> +        else {
> +            chapter_name = demuxer_chapter_name(mpctx->demuxer,
> +                                                chapter);
> +            if (chapter_name) {
> +                snprintf(*(char **) arg, 63, "(%d) %s",
> +                         chapter+1, chapter_name);
> +                free(chapter_name);
> +            }
> +            else {
> +                char tmp[16];
> +                chapter_num = demuxer_chapter_count(mpctx->demuxer);
> +                if (chapter_num <= 0)
> +                    tmp[0] = '\0';
> +                else
> +                    sprintf(tmp, " of %3d", chapter_num);
> +                snprintf(*(char **) arg, 63, "(%d) %s", chapter + 1,
> +                         tmp);
> +            }

This code is duplicated in the libmenu part. Make a new function or add
that to demuxer_chapter_name().

	Albeu