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

[Cyril Russo]

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.

I haven't read the h264's parser, so I absolutely don't know which 
member of those H264's syntax element are extracted.
I *guess* the current H264 parsing code is correct, and *expect* to find 
those element in the FFmpeg Picture::reference and Picture::field_poc, 
as from that part of the code, I don't have access to the initial H264 
NAL to do the parsing myself.

If you know that part of the code, please confirm if it is what is done 
in H264 parsing code.


>
>> I haven't written this code (Gwenole has), I'm only trying to use it and
>> thought it was a shame that only H264 missed in the trunk.
>>     
>
> Thats because the VAAPI-h264 code is a unreadable probably buggy mess, the
> other wrapers where of higher quality
>   
Sure. I understand this.
The lib's VA is quite big, but I don't think it's so badly made as 
you're saying.
They have clearly written that they *needed* a almost empty 
VAPictureH264 before the first slice (a software decoder doesn't) 
containing the (few) common data that the next slice will refer too.
I think they still follow the H264 standard.
We are talking to (limited) "hardware" drivers, and they usually do the 
minimum possible work...

BTW, in all case, as decoding is sent on end_frame, we could fill/merge 
those main stuff while receiving slices. It's more work for each slice 
and it shouldn't be required by std.

>
>> + *  VAAPI requires a fixed size reference picture array.
>>     
>
> well, and what is this supposed to tell us in the doxy of this function?
>   
Nothing, it a bad copy and paste.
Removed.
>
>
>> +static void fill_vaapi_pic(VAPictureH264  *va_pic, 
>> +                           Picture        *pic, 
>>     
>
> trailing whitespace
>
>   
Removed.
>   
>> +                           int             pic_structure)
>> +{
>> +    if (pic_structure == 0)
>> +        pic_structure = pic->reference;
>> +
>> +    va_pic->picture_id = ff_vaapi_get_surface(pic);
>> +    va_pic->frame_idx  = pic->long_ref ? pic->pic_id : pic->frame_num;
>> +
>> +    va_pic->flags = 0;
>> +    if (pic_structure != PICT_FRAME) {
>> +        if (pic_structure & PICT_TOP_FIELD)
>> +            va_pic->flags |= VA_PICTURE_H264_TOP_FIELD;
>>     
>
>   
>> +        if (pic_structure & PICT_BOTTOM_FIELD)
>> +            va_pic->flags |= VA_PICTURE_H264_BOTTOM_FIELD;
>>     
>
> is that equivalent to an else ?
> if so it should be one
>
>   
Unless there's something unintended in mpegvideo.h, we have PICT_FRAME 
== PICT_TOP_FIELD | PICT_BOTTOM_FIELD
That's why I haven't made it a single if (pic_structure == 
PICT_TOP_FIELD) {} else {}

Anyway, I've rewritten the test, considering it's exclusive, please 
check if it's better.
 
>> +    }
>> +    if (pic->reference) {
>>     
>
>   
>> +        if (pic->long_ref)
>> +            va_pic->flags |= VA_PICTURE_H264_LONG_TERM_REFERENCE;
>> +        else
>>     
>
> if{
> }else
> (makes future patches smaller)
>   
Removed.
>
>   
>>      if (pic->picture_id == ff_vaapi_get_surface(ff_pic)) {
>>     
>
>   
>> +            VAPictureH264 new_pic;
>>     
>
> this variable name is poor, we have ff_pic that is a differnt type ...
> really, for the patch to be accepted the vaapi vs. lavc code must be much
> clearer seperated and without any exception
> as is the code is not readable.
>   

> and ff_ is used as a prefix for private global things in lav* its a poor
> choice here as well
>
> and the function above uses pic as a Picture type and va_pic as what pic
> is used here ...
> thats just a mess
>
> [...]
>   
>   
Ok, I guess the code was missing comments, and, you are right, the names 
are confusing.
The current patch should be way better.

fill_vaapi_pic is called at 2 places:
   - in start_frame, where it's only used to track the reference to be 
used later on
   - in decode_slice, where it's really used to fill the VAPictureH264 
members.

The code was made this way to avoid duplicating a similar function, but 
it might be misleading at first.
Let me know if the current patch is good.


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg.hwaccel.vaapi.h264.18.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090609/bf8229ce/attachment.txt>