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

[Diego Biurrun]

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.

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

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

> --- libvo/vo_mng.c	(Revision 0)
> +++ libvo/vo_mng.c	(Revision 0)
> @@ -0,0 +1,698 @@
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <math.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/time.h>
> +#include <sys/mman.h>
> +#include <sys/ioctl.h>
> +
> +#include <zlib.h>

At least sys/config.h is unused, probably more - please trim your
#include list.

> +#define MNG_INCLUDE_WRITE_PROCS
> +#define MNG_ACCESS_CHUNKS
> +#define MNG_SUPPORT_READ
> +#define MNG_SUPPORT_DISPLAY
> +#define MNG_SUPPORT_WRITE
> +#include <libmng.h>
> +
> +#include "config.h"
> +
> +#include "video_out.h"
> +#include "video_out_internal.h"
> +#include "mp_msg.h"
> +#include "m_option.h"
> +#include "fastmemcpy.h"

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

> +static vo_info_t info = {
> +    "MNG driver",
> +    "mng",
> +    "Stefan Schuermans <stefan blinkenarea org>",
> +    ""
> +};

I think the empty initializer is unneeded.

> +typedef struct _vomng_frame {
> +} vomng_frame;
> +
> +typedef struct _vomng_properties_t {
> +} vomng_properties_t;

typedefs on structs are ugly.  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.

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

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

ibmng?

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

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

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

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

unnecessary linebreak

> +    if (mng_putchunk_fram(
> +            h_mng,
> +            MNG_FALSE,
> +            b_first_frame            /* keep canvas if not 1st frame */
> +                ? MNG_FRAMINGMODE_1
> +                : MNG_FRAMINGMODE_NOCHANGE,
> +            0, MNG_NULL,             /* no frame name */
> +            MNG_CHANGEDELAY_DEFAULT, /* change only delay */
> +            MNG_CHANGETIMOUT_NO,
> +            MNG_CHANGECLIPPING_NO,
> +            MNG_CHANGESYNCID_NO,
> +            i_delay_ms, 0,           /* new delay, no new timeout */
> +            0, 0, 0, 0, 0,           /* no new boundary */
> +            0, 0                     /* no count, no IDs */
> +            ) != 0) {

This looks quite ugly, please start the argument list on the same line
as the function call.

more below

> +        mp_msg(MSGT_VO, MSGL_ERR,
> +               "vomng: writing FRAM chunk failed\n");

unnecessary linebreak

more below

> +    if (mng_setcb_openstream (h_mng, vomng_openstream ) != 0 ||
> +            mng_setcb_closestream(h_mng, vomng_closestream) != 0 ||
> +            mng_setcb_writedata  (h_mng, vomng_writedata  ) != 0) {

Indentation is off.

> +    i_duration_ms = vomng->p_frame_last->i_time_ms
> +                  - vomng->p_frame_first->i_time_ms;

  i_duration_ms = vomng->p_frame_last->i_time_ms -
                  vomng->p_frame_first->i_time_ms;

> +static int vomng_prop_setup(void)
> +{
> +    /* create properties structure */
> +    vomng = calloc(1, sizeof (vomng_properties_t));

sizeof(vomng_properties_t)

> +static void
> +vomng_prop_reset(void)

  static void vomng_prop_reset(void)

> +        /* reset state */
> +        vomng->b_is_init = 0;
> +        if (vomng->p_frame_first) {
> +            p_frame = vomng->p_frame_first;
> +            while (p_frame) {
> +                p_next = p_frame->p_next;
> +                if (p_frame->p_data)
> +                    free (p_frame->p_data);
> +                free (p_frame);

free(

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

Align the '='.

> +    /* allocate canvas */
> +    vomng->i_width  = width;
> +    vomng->i_height = height;
> +    row_stride      = 1 + width * 3; /* rows contain filter IDs */
> +    vomng->p_canvas = calloc(height * row_stride, 1);
> +    if (!vomng->p_canvas) {
> +        free (vomng->p_canvas);

free(

> +static int control(uint32_t request, void *data)
> +{
> +    switch (request) {
> +        case VOCTRL_QUERY_FORMAT:
> +            return query_format(*((uint32_t *)data));

Indent case labels at the same depth as the switch (K&R style).

Diego