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

[Stefan Schuermans]

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

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

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

>> +/**
>> + * @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.

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

Best regards,
Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: MPlayer-svn20110516-vomng.diff
Type: text/x-patch
Size: 21940 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110516/969c5523/attachment.bin>