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

[Stefan Schuermans]

Hello Diego.

Diego Biurrun schrieb:
> On Mon, May 09, 2011 at 10:47:58PM +0200, Stefan Schuermans wrote:
>> Diego Biurrun schrieb:
>>> On Sat, May 07, 2011 at 01:01:31PM +0200, Stefan Schuermans wrote:
>>>> --- libvo/vo_mng.c	(Revision 0)
>>>> +++ libvo/vo_mng.c	(Revision 0)
>>>> +mng_bool vomng_writedata(mng_handle h_mng, mng_ptr p_buf,
>>>> +                         mng_uint32 i_size, mng_uint32 *i_written)
>>>> +{
>>>> +  FILE *h_file = (FILE *)mng_get_userdata(h_mng);
>>>> +  *i_written = fwrite(p_buf, 1, i_size, h_file);
>>>> +  return MNG_TRUE;
>>>> +}
>>> This always returns true.
>> Actually, the example code in libmng documentation does it that way. I  
>> guess libmng figures out if everything was written by comparing  
>> *i_written and i_size internally.
> 
> At the very least add a comment that explains why this is done in such
> a braindead way.

That's what I did in the patch I sent yesterday. Sorry that I forgot to 
mention that in the text of my mail.

I'm not sure if this is really braindead or only not very obvious at the 
first glance. fwrite returns only a single value to indicate 
success/error and the number of written items and this value is reported 
back to libmng via *i_written.
As I understood the interface from the libmng documentation, the return 
value of the callback should be false if the fwrite function could not 
be called at all due to some error before, e.g. the file not having been 
opened. Such a thing cannot happen in the easy case we have here, so the 
return value is always true.

>>>> +void vomng_free(mng_ptr ptr, mng_size_t i_size)
>>>> +{
>>>> +  (void)i_size; /* ignored */
>>>> +  free(ptr);
>>>> +}
>>> gcc has an attribute for unused function arguments.  I think we already
>>> use it in other places.
>> I've kept those (void) casts for now, as I saw that there is no common
>> opinion on this list and I personally like this non-gcc-specific way
>> better. However, I'd change that to __attribute__((unused)) if the
>> majority prefers this.
> 
> Why do you mark the unused parameters in the first place?  You could
> skip the void cast, attribute, whatever ...

I did that now.

I usually void-cast unused parameters as some compilers (and also gcc if 
used with both the -W -Wall flags) complain about unused parameters.

>>>> +  /* return allocated compressed data */
>>>> +  *p_out_ptr = out_ptr;
>>>> +  *p_out_len = (unsigned int)out_len;
>>> Such casts make me nervous.
>> I think this cast is safe, due to the behavior of the compress2 call.  
>> I've added comments as explanation.
> 
> But do you need it?

This is a cast I just use to get rid of warnings produced by some 
compilers when assigning to shorter data types. As this is not the case 
for gcc, I removed the cast now.

>> +#include "config.h"
> 
> At least config.h is unused, probably more - please trim your
> #include list.

Done.

>> +static vo_info_t info = {
>> +    "MNG driver",
>> +    "mng",
>> +    "Stefan Schuermans <stefan blinkenarea org>",
>> +    ""
>> +};
> 
> I think the empty initializer is unneeded.

If this pointer is allowed to be NULL, I agree. I searched the source 
and could not find any usage of the comment member of vo_info_t, so I 
guess, NULL is fine although all the other vo modules set it to an empty 
string if they do not provide a comment.

>> +typedef struct _vomng_frame {
>> +} vomng_frame;
>> +
>> +typedef struct _vomng_properties_t {
>> +} vomng_properties_t;
> 
> typedefs on structs are ugly.

I did not know that before. Btw: video_out.h contains a lot of those.

> Plus, the _t namespace is reserved for
> POSIX, don't pollute it further.  Something similar applies to starting
> identifiers with '_', which is reserved at the file level.  You are not
> misusing that here, but let's avoid it before it spreads.

Fixed.

>> +/**
>> + * @brief libmng callback: open stream
>> + * \pram[in] h_mng libmng handle
>> + * @return if stream could be opened
>> + */
> 
> Hmmm, \pram is broken - please run 'make doxygen' and make sure there
> are no warnings or errors for your file.

Oh, that's a rather obvious one. Sorry I did not see that. Now I also 
ran 'make doxygen' and got no warnings from my files.

>> +/**
>> + * @brief ibmng callback: stream should be closed
>> + * @param[in] h_mng libmng handle
>> + * @return if stream could be closed
>> + */
> 
> ibmng?

Missing "l" added.

>> +/**
>> + * @brief libmng callback: write libmng data to the open stream
>> + * @param[in] h_mng libmng handle
>> + * @param[in] *p_buf pointer to data to write
>> + * @param[in] i_size size of data to write
>> + * @param[out] *i_written size of data written
>> + * @return is data was written successfully
>> + */
> 
> The @return statement makes no sense.

It it now "if data was ...".

>> +static void vomng_canvas_to_compressed(unsigned int i_width,
>> +                                       unsigned int i_height,
>> +                                       const unsigned char *p_canvas,
>> +                                       unsigned char **p_out_ptr,
>> +                                       unsigned int *p_out_len)
>> +{
>> +    unsigned int raw_len;
>> +    unsigned char *out_ptr;
>> +    unsigned long out_len;
>> +
>> +    /* default: no output */
>> +    *p_out_ptr = NULL;
>> +    *p_out_len = 0;
>> +
>> +    /* length of input data */
>> +    raw_len = i_height * (i_width + 1) * 3; /* rows contain filter IDs */
>> +                                            /* raw_len will be significantly
>> +                                                smaller than UINT_MAX */
>> +
>> +    /* compress data */
>> +    out_len = raw_len                      /* must be at least */
>> +            + (raw_len + 999) / 1000 + 12; /* <source length> * 1.001 + 12 */
>> +                                           /* out_len will be smaller than
>> +                                              UINT_MAX */
> 
> I think these comments would make more sense above the code
> instead of right of it.

Okay.

>> +    out_ptr = malloc(out_len);
>> +    if (!out_ptr)
>> +        return;
>> +    compress2(out_ptr, &out_len, p_canvas, raw_len, Z_BEST_COMPRESSION);
>> +
>> +    /* return allocated compressed data */
>> +    *p_out_ptr = out_ptr;
>> +    *p_out_len = (unsigned int)out_len; /* cast is safe, compress2 never
>> +                                           increases value of out_len */
> 
> But is the cast needed?  Why not make out_len the right type to begin
> with?

The "problem" is that compress2 returns the length as unsigned long and 
libmng wants the compressed data length as a shorter type. So the type 
has to become smaller somewhere. I chose to do the shortening in this 
fuction, as it is understandable there that the value in out_len is 
actually not large enough to cause problems.

I now converted my frame structure to libmng data types, so I can 
directly convert to the final type here. I also explain the issue in 
comments above the code now.

>> +    if (mng_putchunk_seek(h_mng, 0, MNG_NULL) != 0) {
>> +        mp_msg(MSGT_VO, MSGL_ERR,
>> +               "vomng: writing SEEK chunk failed\n");
> 
> unnecessary linebreak
>
> [...]

I fixed this and the other broken formatting/indenting issues.

Regards,
Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: MPlayer-svn20110510-vomng.diff
Type: text/x-patch
Size: 23411 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110510/ee443034/attachment-0001.bin>