[MPlayer-dev-eng] [PATCH] MNG output support for MPlayer

[Clément Bœsch]

On Mon, May 16, 2011 at 10:01:36PM +0200, Stefan Schuermans wrote:
> Hi Clément,
> 
> Clément Bœsch schrieb:
> >On Sun, May 15, 2011 at 09:31:54PM +0200, Stefan Schuermans wrote:
> >[...]
> >>As I'm sending a new version of the patch anyway, I also changed all
> >>standalone prefix increments to postfix increments.
> >
> >I just tested it and it seems to work, at least a file is generated.
> >Thought, I wasn't able to "play" it correctly… Maybe I'm missing
> >something, but since there is no documentation I'm a bit lost :)
> 
> Maybe there is still some hidden problem either in this patch or in
> the MNG demuxer?! Please try the commands below.
> 
> >If you can give me a hint on how to play it to show me it works, I'm OK
> >committing it. Currently I'm just able to have a static image with ffplay.
> 
> ./mplayer -vo mng:turn-on-off.mng \
> http://samples.mplayerhq.hu/MPEG-4/turn-on-off.mp4
> ./mplayer turn-on-off.mng
> 

I confirm it works. Retested with my initial test too. I don't know what I
messed.

I'll apply the last version of your patch in 3 days if no one object.

> >Thought, I'm attaching yet another notes on things I missed.
> >
> >>+static vo_info_t info = {
> >>+    .name       = "MNG driver",
> >
> >MNG file
> 
> done
> 
> >>+/* a frame to be written to the MNG file */
> >>+struct vomng_frame {
> >>+    mng_ptr data;             /* deflate compressed data, malloc-ed */
> >>+    mng_uint32 len;           /* length of compressed data */
> >>+    unsigned int time_ms;     /* timestamp of frame (in ms) */
> >>+    struct vomng_frame *next; /* next frame */
> >>+};
> >>+
> >
> >Can you use Doxygen comments instead?
> >
> >///< deflate...
> 
> Okay, but I prefer /**< deflate ... as this is a C file and // is C++.
> 

As you wish, Diego will complain if he doesn't agree anyway. Btw, // is
C99 too, and it is used in various places.

> >>+/* properties of MNG output */
> >>+struct vomng_properties {
> >>+    char               *out_file_name; /* name of output file, malloc-ed */
> >
> >About the "malloc-ed", see below.
> 
> I think it's needed, see below.
> 
> >>+/* private data of MNG vo module */
> >>+static struct vomng_properties *vomng = NULL;
> >>+
> >
> >Pointless init.
> 
> I converted the context to a static global variable (see below), so
> this initialization is gone.
> 

Good. This is better, and it allows easier potential future suboption
integration.

> >>+/**
> >>+ * @brief set up properties of MNG vo module
> >>+ * @return 0 on success, 1 on error
> >>+ */
> >>+static int vomng_prop_setup(void)
> >>+{
> >>+    /* create properties structure */
> >>+    vomng = calloc(1, sizeof(struct vomng_properties));
> >
> >sizeof(*vomng);
> 
> done
> 
> >>+    /* 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_prop_cleanup();
> >>+        return 1;
> >>+    }
> >>+    if (vomng->out_file_name)
> >>+        free(vomng->out_file_name);
> >
> >Pointless if.
> 
> removed
> 
> >>+    vomng->out_file_name = strdup(arg);
> >
> >Are you see you really need to reallocate the string?
> 
> If I just keep a copy of the arg pointer I get very strange
> filenames that usually consist out of a couple of control
> characters.
> 

Mmmh ok. Anyway, be aware of the suboption system if you need to add
options. It will do all the allocations you need.

> >Also, this is good practice to dynamically allocate a context but I wonder
> >if it was worst the effort. Anyway, doesn't matter much.
> 
> I took another look at some other vo modules. For example vo_png
> does not allocate the stuff dynamically, so I also converted the
> context to a static global variable.
> 

Yes, thanks.

-- 
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/20110518/cbeb2829/attachment.asc>