[MPlayer-dev-eng] [PATCH] MNG default output filename

[Clément Bœsch]

On Mon, May 30, 2011 at 08:23:43PM +0200, Stefan Schuermans wrote:
> on 30.05.2011 17:13, Diego Biurrun wrote:
> >On Sat, May 28, 2011 at 12:00:57AM +0200, Stefan Schuermans wrote:
> >>--- DOCS/man/en/mplayer.1	(Revision 33506)
> >>+++ DOCS/man/en/mplayer.1	(Arbeitskopie)
> >>@@ -4790,8 +4790,29 @@
> >> .
> >>+.TP
> >>+.B "mng\ \ \ \ "
> >>+Output each frame into a single animated MNG file.
> >>+It supports only RGB format with 24 bpp.
> >>+The frames in the output are compressed losslessly.
> >>+.PD 0
> >>+.RSs
> >>+.IPs <output>
> >>+Specify the output filename (mandatory).
> >
> >Sorry for not noticing earlier, but this is bad.
> >All other (image) video output drivers do use a
> >default filename, mng should not be an exception.
> 
> No problem, please see my proposed fix.
> 

Sorry for the late review.

> Index: libvo/vo_mng.c
> ===================================================================
> --- libvo/vo_mng.c	(Revision 33526)
> +++ libvo/vo_mng.c	(Arbeitskopie)
> @@ -40,6 +40,7 @@
>  #include "video_out.h"
>  #include "video_out_internal.h"
>  #include "mp_msg.h"
> +#include "subopt-helper.h"
>  
>  #define VOMNG_DEFAULT_DELAY_MS (100) /* default delay of a frame */
>  
> @@ -566,6 +567,14 @@
>      return 0;
>  }
>  
> +/** list of suboptions */
> +static const opt_t subopts[] = {
> +    {   .name = "output",
> +        .type = OPT_ARG_MSTRZ,
> +        .valp = &vomng.out_file_name },
> +    {   .name = NULL }
> +};
> +

Looks a bit overkill to specify attributes in this case, and style looks
weird, but I'm ok with it. Please stick to what you see in the rest of the
code next time.

>  /**
>   * @brief pre-initialize MNG vo module
>   * @param[in] *arg arguments passed to MNG vo module (output file name)
> @@ -573,19 +582,14 @@
>   */
>  static int preinit(const char *arg)
>  {
> -    /* get name of output file */
> -    if (!arg || !*arg) {
> -        mp_msg(MSGT_VO, MSGL_ERR, "vomng: MNG output file must be given,"
> -                                  " example: -vo mng:output.mng\n");
> +    vomng.out_file_name = strdup("output.mng");
> +

Why don't you alloc out_file_name in case it is NULL after the
subopt_parse? It doesn't really matter since the buffer is free because of
OPT_ARG_MSTRZ, but the code doesn't look obvious that way IMO.

Also, please use "out.mng" just like vo gif.

> +    if (subopt_parse(arg, subopts)) {
> +        mp_msg(MSGT_VO, MSGL_ERR, "vomng: invalid MNG suboptions,"
> +                                  " example: -vo mng:output=output.mng\n");

Do you mind using the same output as vo_gif? "Example: mplayer ..."

Looks good enough for me otherwise. I let Diego take over.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110611/0b66885b/attachment.asc>