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

[Diego Biurrun]

On Sat, May 07, 2011 at 01:01:31PM +0200, Stefan Schuermans wrote:
>
> in 2008, I implemented MNG input support for MPlayer. I was now in need
> of creating MNG files from short MPEGs and have implemented an MNG vo
> plugin for MPlayer. I've attached the patch for review.
>
> --- libvo/vo_mng.c	(Revision 0)
> +++ libvo/vo_mng.c	(Revision 0)
> @@ -0,0 +1,702 @@
> +
> +#define VOMNG_DEFAULT_DELAY_MS (100) /* default delay of a frame */
> +
> +static vo_info_t info = {
> +  "MNG driver",
> +  "mng",
> +  "Stefan Schuermans <stefan blinkenarea org>",
> +  ""
> +};

We use 4 spaces indentation for new files.

> +/* prototypes */
> +mng_ptr vomng_alloc(mng_size_t i_size);
> +void vomng_free(mng_ptr ptr, mng_size_t i_size);
> +mng_bool vomng_openstream(mng_handle h_mng);
> +mng_bool vomng_closestream(mng_handle h_mng);
> +mng_bool vomng_writedata(mng_handle h_mng, mng_ptr p_buf,
> +                         mng_uint32 i_size, mng_uint32 *i_written);
> +void *vomng_output(void *param);

You should rather make the functions static.

> +/**
> + * \brief libmng callback: allocate memory
> + * \param[in] i_size size of requested memory block
> + * \return pointer to memory block, which is initialized to zero
> + */

Replace the '\' by '@' in your Doxygen.

> +mng_ptr vomng_alloc(mng_size_t i_size)
> +{
> +  void * ptr = malloc(i_size);
> +  if(ptr != NULL)
> +    memset(ptr, 0, i_size);
> +  return (mng_ptr)ptr;
> +}

calloc

> +/**
> + * \brief libmng callback: free memory
> + * \param[in] pointer to memory block
> + * \param[in] i_size size of requested memory block
> + */
> +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.

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

Also - is the cast necessary?

> +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 */
> +
> +  /* compress data */
> +  out_len = raw_len                      /* must be at least */
> +          + (raw_len + 999) / 1000 + 12; /* <source length> * 1.001 + 12 */
> +  out_ptr = (unsigned char *)malloc(out_len);

pointless void* cast

> +  /* return allocated compressed data */
> +  *p_out_ptr = out_ptr;
> +  *p_out_len = (unsigned int)out_len;

Such casts make me nervous.

> +      vomng->p_frame_first = NULL;
> +      vomng->p_frame_last = NULL;
> +    }

nit: align the '=', more below

> +    vomng->i_width = 0;
> +    vomng->i_height = 0;
> +
> +  }

Drop the pointless empty line.

> +  /* create new frame */
> +  p_frame = (vomng_frame *)calloc(1, sizeof(vomng_frame));

pointless void* cast

Diego