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

[Ulion]

2007/12/10, Alban Bedel <albeu at free.fr>:
> 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).

Oh, I thought a property must have a command, that's not true, then I
will remove the chapter command. But the property still need a MP_CMD
id, should I use a new one MP_CMD_CHAPTER in current patch, or use the
existed one?

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

Sure.

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

Good idea, now added a demuxer_chapter_display_name function in demuxer.

Updated patches is here.
demuxer: add chapter_display_name function.
property & command: fix chapter_name memory leak at property function
end. using chapter_display_name function for property print. remove
chapter command.
libmenu: using chapter_display_name function. use seek_chapter command
instead of removed chapter command.

-- 
Ulion
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: demuxer_chapter_funcs2.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071210/19b1008d/attachment.asc>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: chapter_property2.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071210/19b1008d/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: osdmenu_chapter2.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071210/19b1008d/attachment-0001.asc>