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

[Giorgio Vazzana]

2012/1/21 Alexander Strasser <eclipse7 at gmx.net>:
>  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.

Done.

> [...]
>> @@ -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.

Done.

> [...]
>> -    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?

Yes. I had sent a patch to add that message a while back, and an extra
":" had slipped in, so I thought I'd fix it now.

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

New patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-theora-uncrustify.diff
Type: text/x-patch
Size: 7681 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20120121/11ab9bdd/attachment.bin>