[MPlayer-dev-eng] [PATCH] Do not call to th_decode_ycbcr_out() for packets representing a dropped (0-byte) frame.

[Alexander Strasser]

Hi,

Giorgio Vazzana wrote:
> 2012/1/19 Diego Biurrun <diego at biurrun.de>:
> > On Wed, Jan 11, 2012 at 01:35:52PM +0100, Giorgio Vazzana wrote:
[...]
> >> After this patch, the file needs a reindent, I don't know if there is
> >> an automated program/script that can do it. If not let me know and
> >> I'll do it by hand, after all it's a small file. Regards,
> >
> > TOOLS/mp-uncrustify-style.cfg
> >
> > is our configuration file for uncrustify.  You can run that and make
> > a few corrections by hand.
> 
> Done, see attached patch.

  Looks mostly fine. Just a few remarks:

> Index: libmpcodecs/vd_theora.c
> ===================================================================
> --- libmpcodecs/vd_theora.c	(revision 34581)
> +++ libmpcodecs/vd_theora.c	(working copy)
[...]
> @@ -45,30 +45,32 @@
>  
>  typedef struct theora_struct_st {
>      th_setup_info *tsi;
> -    th_dec_ctx    *tctx;
> -    th_comment     tc;
> -    th_info        ti;
> +    th_dec_ctx *tctx;
> +    th_comment tc;
> +    th_info ti;
>      th_ycbcr_buffer ycbcrbuf;
>  } theora_struct_t;

  Optionally you can align this so it matches with ycbcrbuf. The tabular
layout is a bit easier to scan when skimming over the source code. Choose
whatever you prefer though.

[...]
> @@ -77,15 +79,16 @@
>  /*
>   * init driver
>   */
> -static int init(sh_video_t *sh){
> +static int init(sh_video_t *sh)
> +{
>      theora_struct_t *context = NULL;
> -    uint8_t *extradata = (uint8_t *)(sh->bih + 1);
> -    int extradata_size = sh->bih->biSize - sizeof(*sh->bih);
> +    uint8_t *extradata       = (uint8_t *)(sh->bih + 1);
> +    int extradata_size       = sh->bih->biSize - sizeof(*sh->bih);
>      int errorCode = 0;

  Optionally you may consider aligning the extradata/extradata_size
variable names and the initializer of errorCode to make it look less
haphazard. Or probably don't align anything at all and just use one
space everywhere here. Or leave as you have it now if you prefer.

[...]
> -    mp_msg(MSGT_DECVIDEO,MSGL_V,"INFO: Theora video init ok!\n");
> -    mp_msg(MSGT_DECVIDEO,MSGL_INFO,"Frame: %dx%d, Picture %dx%d, Offset [%d,%d]\n", context->ti.frame_width, context->ti.frame_height, context->ti.pic_width, context->ti.pic_height, context->ti.pic_x, context->ti.pic_y);
> +    mp_msg(MSGT_DECVIDEO, MSGL_V, "INFO: Theora video init ok!\n");
> +    mp_msg(MSGT_DECVIDEO, MSGL_INFO, "Frame %dx%d, Picture %dx%d, Offset [%d,%d]\n", context->ti.frame_width, context->ti.frame_height, context->ti.pic_width, context->ti.pic_height, context->ti.pic_x, context->ti.pic_y);

  This is the only (minor) semantic difference I could spot. Did you
remove the ':' in 'Frame:' intentionally? The message seems more
consistent with that change, so I think you changed it on purpose and
I think it is OK.

  Although the message is at MSGL_INFO, I don't assume anyone relied
on it when parsing mplayer output.


[...]

  Alexander