[FFmpeg-devel] [PATCH][VAAPI][6/6] Add H.264 bitstream decoding (take 15)
Michael Niedermayer
michaelni
Thu Jun 4 00:19:23 CEST 2009
On Tue, Jun 02, 2009 at 04:03:50PM +0200, Cyril Russo wrote:
> Michael Niedermayer a ?crit :
>>
>> please document the convention used for function names
>> which are part of our wraper and which are part of VAAPI
>> In that sense is it really needed to have h264 and vaapi prefixes for
>> static
>> function names in a file that is specific to these?
>>
>>
> I've removed the prefix for the static functions used to convert the data
> type between FFmpeg and VAAPI.
>
> The h264 prefix are modified to vaapi_h264 to be consistent with other
> vaapi_*.c files. (vaapi_vc1, vaapi_mpeg4 and others).
> I've added a single line below the copyright notice to explain what the
> file's code does.
>
> I think it's obvious from other vaapi_'s files, but I'm not the best guy to
> judge.
>>
>>> +{
>>> + va_pic->picture_id = 0xffffffff;
>>> + va_pic->flags = VA_PICTURE_H264_INVALID;
>>> + va_pic->TopFieldOrderCnt = 0;
>>> + va_pic->BottomFieldOrderCnt = 0;
>>> +}
>>> +
>>>
>>
>>
>>> +/** Translate an FFmpeg Picture into its VAAPI form.
>>> + * @param[out] va_pic A pointer to VAAPI's own picture struct
>>> + * @param[in] pic A pointer to the FFmpeg picture struct
>>> to convert
>>>
>>
>>
>>> + * @param[in] pic_structure The picture field type to use, as
>>> defined in mpegvideo.h
>>> + * (can be 0 to use pic's field type) */
>>>
>>
>> what does "to use" mean? I dont think thats clear
>>
> Reformulated.
> During conversion, the pic_structure parameter can be used instead of the
> pic's internal field type (as VAAPI is not following the same format as
> FFmpeg).
>>
>>
>>> +static void vaapi_h264_fill_vaapi_picture(VAPictureH264 *va_pic,
>>>
>>
>> the function name is not good
>>
>> libav_pic_to_vaapi()
>> might be reasonable
>>
>> [...]
>>
> There isn't any "libav_" in the code repository, and "av_" look like it's
> public in the other files.
> I've modified all functions' name like this one to "fill_something"
> (fill_reference_frame_array, etc...), and added a comment on the top of the
> file.
>
>>
>>
>>> +/** Initialize VAPictureParameterBufferH264.ReferenceFrames[] array
>>> + * from its FFmpeg counterpart. */
>>>
>> [...]
>>
>>> +/** Initialize VAAPI reference picture lists from the FFmpeg reference
>>> picture list.
>>>
>>
>> hmm, confusing
>>
>> [...]
>>
> While the former isn't clear, the latter seems logical to me.
> VAAPI and FFmpeg are different code, and we must convert some of the data
> from FFmpeg to VAAPI.
> Let me know if the attached patch is better.
[...]
> +/** Initialize an empty VAAPI picture.
> + * VAAPI requires a fixed size reference picture array. */
> +static void init_picture(VAPictureH264 *va_pic)
init_vapic(ture)
> +{
> + va_pic->picture_id = 0xffffffff;
> + va_pic->flags = VA_PICTURE_H264_INVALID;
> + va_pic->TopFieldOrderCnt = 0;
> + va_pic->BottomFieldOrderCnt = 0;
> +}
> +
> +/** Translate an FFmpeg Picture into its VAAPI form.
> + * @param[out] va_pic A pointer to VAAPI's own picture struct
> + * @param[in] pic A pointer to the FFmpeg picture struct to convert
> + * @param[in] pic_structure The picture field type (as defined in mpegvideo.h),
> + * supersede pic's field type if nonzero. */
this is much better, thanks
> +static void fill_vaapi_pic(VAPictureH264 *va_pic,
> + Picture *pic,
> + int pic_structure)
the function name is inconsistant to init_picture() (vaapi_pic vs picture)
[...]
> +/** Fill VAAPI reference picture lists.
> + * @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 and */ should be on a line of its own because that makes
future patches smaller (they dont have to move it down)
> +static void fill_reference_pic_list(VAPictureH264 RefPicList[32],
[...
> +static void h264_fill_plain_pred_weight_table(H264Context *h, int list,
[...]
> +
> +/** Initialize and start decoding a frame with VAAPI. */
> +static int vaapi_h264_start_frame(AVCodecContext * avctx,
i must say the prefixes still are entirely random
vaapi_h264_ here h264_ above and nothing above that
it all would be fine where there a difference but its all h264 and vaapi
related code.
[...]
> +/** End a hardware decoding based frame. */
sounds a little un-english
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
There will always be a question for which you do not know the correct awnser.
-------------- 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/20090604/5644f078/attachment.pgp>
More information about the ffmpeg-devel
mailing list