[FFmpeg-devel] [PATCH][VAAPI][6/6] Add H.264 bitstream decoding (take 7)

[Michael Niedermayer]

On Fri, Apr 17, 2009 at 10:05:30AM +0200, Gwenole Beauchesne wrote:
> On Fri, 17 Apr 2009, Michael Niedermayer wrote:
>
>>>>> +    pic_param->bit_depth_luma_minus8                            =
>>>>> h->sps.bit_depth_luma   >= 8 ? h->sps.bit_depth_luma   - 8 : 0;
>>>>> +    pic_param->bit_depth_chroma_minus8                          =
>>>>> h->sps.bit_depth_chroma >= 8 ? h->sps.bit_depth_chroma - 8 : 0;
>>>>
>>>> it cant be smaller 8 (not counting overflows on broken streams)
>>>
>>> Mmmm, I think I had a case where it was needed, I will check again with 
>>> my
>>> samples tomorrow.
>
> It works so far, so let's just stick to this newer patch.
>
>>> +    assert(dpb->size < dpb->max_size);
>>> +    vaapi_h264_fill_picture(&dpb->pics[dpb->size++], ff_pic, 0);
>>
>> i think this assert should be a hard check as its failure looks
>> bad
>
> OK, changed that function return value to int and propagated it to 
> vaapi_h264_fill_ReferenceFrames().
>
>>> +        *p_luma_weight_flag   = h->luma_weight_flag[list];
>>> +        *p_chroma_weight_flag = h->chroma_weight_flag[list];
>>
>> and whats the sense of this double indirection?!
>
> For code simplification but it seems to be a failure. ;-) So, I have now 
> written a specific function, as in the new attachment. WDYT?


[...]
> +static void dpb_init(DPB *dpb, VAPictureH264 *pics, unsigned int max_size)
> +{
> +    unsigned int i;
> +
> +    dpb->size     = 0;
> +    dpb->max_size = max_size;
> +    dpb->pics     = pics;
> +    for (i = 0; i < dpb->max_size; i++)
> +        vaapi_h264_init_picture(&dpb->pics[i]);
> +}

you could inline this, its small its caller is small and its used just once


[...]

> +                                              unsigned char *luma_weight_flag,
> +                                              short          luma_weight[32],
> +                                              short          luma_offset[32],
> +                                              unsigned char *chroma_weight_flag,
> +                                              short          chroma_weight[32][2],
> +                                              short          chroma_offset[32][2])
> +{

> +    const int luma_def   = 1 << h->luma_log2_weight_denom;
> +    const int chroma_def = 1 << h->chroma_log2_weight_denom;

they are just used once and not in any complex code

[...]

also maybe you could add a single sentance to each function to describe
what it does.


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090418/3e6e78c6/attachment.pgp>