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

[Michael Niedermayer]

On Tue, Jun 09, 2009 at 06:19:56PM +0200, Cyril Russo wrote:
> Michael Niedermayer a ?crit :
>>
>>> There are 3 of them in each files, by the end of the file, so they can
>>> be located quite easily.
>>> Unless you expressely say to rename them, I'd prefer to keep them that 
>>> way.
>>>     
>>
>> rename the ones in this patch, the other files can be dealt with at some
>> point in the future ...
>>
>>
>>   
> done
>>> I've completed the initial comment to state so.
>>>
>>>     
>>>>> +/** Fill VAAPI's reference frames array. */
>>>>>             
>>>> [...]
>>>>
>>>>         
>>>>> +/** Fill VAAPI reference picture lists.
>>>>>             
>>>> what is the difference between the 2 ?
>>>>
>>>>
>>>>         
>>> The picture isn't the frame (...you know this..., so I guess the real
>>> question was "Why is there a difference between the 2?")
>>>     
>>
>> no, really i think the doxy assumes that a reader is familiar with the
>> definitions of frame and picture (from MPEG) as well as VAAPI and H.264
>> in other words i think it could be a little more verbose
>>
>>
>>   
> see below
>>> Generally speaking, the reference frame for VAAPI is linked to a
>>> hardware buffer by an index.
>>> As specified elsewhere in the file, VAAPI doesn't infer any value, so it
>>> must have access to all the data for its operations (even defaults)
>>>
>>> The former takes a FFmpeg's H264 context, and for each picture with a
>>> (nonzero) reference, create a reference frame entry in the VAAPI ref's
>>> array.
>>> It is only used in the start_frame function once.
>>>     
>>
>> the function uses variables that differ between slices, i doubt that that
>> works except by luck that they tend not to differ in practice.
>>
>>
>>   
> I disagree here:
>
> ITU H264 states (p101):
>
> 7.4.3 Slice header semantics
> When present, the value of the slice header syntax elements 
> pic_parameter_set_id, frame_num, field_pic_flag,
> bottom_field_flag, idr_pic_id, pic_order_cnt_lsb, 
> delta_pic_order_cnt_bottom, delta_pic_order_cnt[ 0 ],
> delta_pic_order_cnt[ 1 ], sp_for_switch_flag, and slice_group_change_cycle
> *shall* be the same in all slice headers of a coded picture.
>
>
> The function only want to know if the next slices will be top or bottom 
> field (infered from from H264's bottom_field_flag), and if it's a 
> short/long term reference (inferred from H264's frame_num).
> It also extract the top/bottom field order count (inferred from H264 
> delta_pic_order_cnt[0] and [1])
> Don't be mislead by the fill_vaapi_pic function that is (re)used to really 
> fill an *entire* VAAPI pic later in decode_slice.

prepare_vaapi_reference_frame_array() uses list_count and ref_count, these
are per slice not per frame values while the function is called per frame
your explanation does not seem to explain this discrepancy ...

[...]
> +/** @file 
> + *  This file implements the glue code between FFmpeg's and VAAPI's structures.
> + */
> +

> +/** Reconstruct bitstream slice_type. */

please place the */ on a line of its own, it makes adding
more lines later look cleaner in diffs because they dont
need to change the first line

[...]
> +/** Decoded picture buffer. */

same thing about */


[...]
> +static int append_to_vaapi_decoded_pic_buf(DPB *dpb, Picture *pic)
> +{
> +    unsigned int i;
> +
> +    if (dpb->size >= dpb->max_size)
> +        return -1;
> +
> +    for (i = 0; i < dpb->size; i++) {
> +        VAPictureH264 * const merged_pic = &dpb->pics[i];
> +
> +        /* Check if the reference picture was already refered in a previous pass */
> +        if (merged_pic->picture_id == ff_vaapi_get_surface(pic)) {
> +            /* Don't overwrite our reference, use a temporary */
> +            VAPictureH264 tmp_pic;

i still am not happy about calling lavc and VAs pictures pic,


> +            fill_vaapi_pic(&tmp_pic, pic, 0);
> +            if ((tmp_pic.flags ^ merged_pic->flags) & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD)) {
> +                /* Merge second field. */
> +                merged_pic->flags |= tmp_pic.flags & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
> +                if (tmp_pic.flags & VA_PICTURE_H264_TOP_FIELD) {
> +                    merged_pic->TopFieldOrderCnt    = tmp_pic.TopFieldOrderCnt;
> +                } else {
> +                    merged_pic->BottomFieldOrderCnt = tmp_pic.BottomFieldOrderCnt;
> +                }
> +            }
> +            return 0;
> +        }
> +    }

i think readability could benefit from empty lines

[...]

> +/** Fill VAAPI reference picture lists.
> + *  Called for each field in slice, it fills the reference picture
> + *  list for that field. 
> + *  @param[out] RefPicList  VAAPI internal reference picture list
> + *  @param[in]  ref_list    A pointer to the FFmpeg reference list
> + *  @param[in]  ref_count   The number of reference pictures in ref_list

> + */    

trailing whitespace


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20090609/52233489/attachment.pgp>