[MPlayer-dev-eng] [PATCH] MNG output support for MPlayer
Diego Biurrun
diego at biurrun.de
Tue May 10 11:44:01 CEST 2011
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
More information about the MPlayer-dev-eng
mailing list