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

[Michael Niedermayer]

On Wed, Jun 10, 2009 at 09:55:39AM +0200, Cyril Russo wrote:
>
>>> 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 ...
>>
>> [...]
>>   
> If I understand H264.c code correctly,
> decode_frame (7677) calls:
> decode_nal_units (7459) that calls:
> decode_slice_header (3704) that *fills* the ref_count and list_count 
> (around line 3980), just before calling hwaccel->start_frame.
>
> I think the H264 code is correct here (wrt the standard) (in that it only 
> decodes slice *header*).
> HWAccel::start_frame is called after the first slice is parsed (it isn't 
> wrong by itself, I can't figure how it could be differently).
> After this case, HWAccel::decode_slice is called for each other slice.
>
> Then, either we interpret the VAAPI doc strictly as it's written:
>
> "For each picture, and before any slice data, a single picture parameter 
> buffer must be send."
>
> In that case, it only needs one of those VAPictureH264 (with the ref only 
> for the first slice) in the start_frame.
>
> Either we intepret the VAAPI doc as what you're suggesting, where each 
> slice accumulate in the ref array that is send first (in that case, I need 
> to merge the ref in decode_slice function).
> I think the former is the intended case, even if, I agree, it isn't a very 
> smart choice.

i dont suggest anything,
the code and the comments have no relation
the per frame code uses per slice fields that are not valid for more
than a single slice, this looks like a serious bug. If it is not so it has
to be elaboretly justified and documented, not hidden as it is.

What you do is like saying the first name of the first page describes the
phone book in which it is written. I hope this makes it clear where the
issue is.

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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- 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/20090610/8a657b7c/attachment.pgp>