[FFmpeg-devel] [PATCH][VAAPI][6/6] Add H.264 bitstream decoding (take 18)
Cyril Russo
stage.nexvision
Tue Jun 9 20:43:51 CEST 2009
Michael Niedermayer a ?crit :
> 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 ...
>
I guess, I'll have to dig into lavc H264 parser to find out where those
syntax el are parsed.
Do you have any shortcut doc to those values ? (Sorry, I'm not a H264
NAL expert, I don't want to misinterpret the code).
Where can I find per frame data (about common slice headers ?)
> [...]
>
>> +/** @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,
>
I don't get it.
Everywhere in the file, lavc use "pic".
Even in the quoted code fragment, pic is lavc, and vapic is anything
else (I'm using merged_pic, or va_pic)
I thought using merged_pic here would be clearer than a unmeaningful
va_pic (same for tmp_pic).
Anyway, how would you name the variable ?
>
>
>> + 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
>
So do I.
I'll send an updated patch tomorrow morning.
> [...]
>
>
>> +/** 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
>
>
> [...]
>
I've run the file to "sed 's/[ \t]*$//'", but it doesn't seems to work.
I'll remove them by hand.
> ------------------------------------------------------------------------
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list