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

[Ulion]

2007/12/11, Andrew Calkin <calkina at geexbox.org>:
> 2007/12/11, Ulion <ulion2002 at gmail.com>:
> >
> > By now I keep the property named 'chapter', it make me feel better.
> > About what you said in the menu directly use chapter property, using
> > property is a little complex than directly use the seek_chapter
> > command, so I keep to use the command.
> >
> > Here's the updated patches:
> > demuxer: add a functiond demuxer_chapter_time to get chapter start and
> > end time if available. And a little chapter name format adjust.
> > property & command: property chapter use same cmd id with command
> > seek_chapter and handle all requests to command seek_chapter, old
> > command handler for MP_CMD_SEEK_CHAPTER removed.
> > libmenu: add a keybinding entry for chapsel in menu.conf. support
> > display start time info in chapter menu and little custom format
> > supported.
>
> I had a quick look through:
>
> osdmenu_chapter3.diff:
>
> in libmenu/menu_chapsel.c, is:
>
> +#include "stream/stream.h"
>
> required anymore? It doesn't seem you use it, so probably legacy from
> when stream was accessed directly.

It is needed for compiling, there's some struct in demuxer.h need it.


>
> In fmt_replace(), why is p declared "const char *p"? I cannot see why
> it is declared const, and also not sure if it is ok to call it const?
> I guess someone more knowledgable can help with an explanation.

Oh, it's my fault, fixed.

>
> Minor nit, in fill_menu(), you ensure that start_time >=0, so
> timestr[9] should suffice, right? Perhaps a warning if hour > 99, as
> it would indicate a bizarre file where the chapter information it
> returns for the menu is bogus.

Take it easy, it's just a local stack variable, indeed now I make it
size 13 to make me feel safer with any start_time within 2147483648.

>
> In read_cmd(), even though cmdbuf[26] is quite large still no overrun
> checking is performed. Perhaps snprintf(cmdbuf, 25...) and
> cmdbuf[25]='\0'; to ensure that a bogus file chapter index doesn't
> cause a buffer overrun. I didn't trace all the way through the code,
> so I'm not sure if it's an issue or not.

I don't see there's such needs, an cid is greater than 0, and max be
the chapter_num, even if chapter_num is a large integer that takes
maximum 10 bytes to be displayed, the lefted buffer can still hode a
'\0' at the end. Or am I wrong?

Updated patch No.3 is here.

-- 
Ulion
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: osdmenu_chapter4.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071211/df93760f/attachment.txt>