[FFmpeg-devel] [PATCH]lavu/hwaccel_vaapi: Add a fixme for the missing byte_order check

[Mark Thompson]

On 05/10/16 19:02, Carl Eugen Hoyos wrote:
> 2016-10-05 16:39 GMT+02:00 Mark Thompson <sw at jkqxz.net>:
>> On 05/10/16 13:06, Carl Eugen Hoyos wrote:
>>>
>>> I cannot test here but afaict, the current implementation of
>>> vaapi_pix_fmt_from_fourcc() can't be correct.
> 
>>> +// FIXME: Take VAImageFormat->byte_order into account
>>>  static struct {
>>>      unsigned int fourcc;
>>>      unsigned int rt_format;
> 
>> Have you found / had reported to you some case which
>> causes a problem here?  If so, please could you offer some
>> more detail about it (especially the driver being used).
> 
> No, I wouldn't even know how to use the code in question.
> 
>> In the cases I know of, the VAImageFormat structures are
>> are all hard-coded such that the fourcc is really the only
>> meaningful information in them:
> 
> So the byte_order field in VAImageFormat has no meaning
> and is completely useless?

Mostly yes, I think.  As you observe in the links below, it is hard-coded in the two main drivers to be either undefined or VA_LSB_FIRST.

>> <https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/va/image.c#n41>
> 
> This link was the reason for my mail.
> 
>> * <https://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c#n246>
> 
> Not sure if this makes sense...

Looking at <https://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c#n1682> at the same time might make the use of that table clearer.

>> This is reflected by the use in hwcontext_vaapi.c, which only
>> fetches the driver's version of the structure in order to pass
>> it back in vaCreateImage() calls - the other fields are never
>> touched at all.
> 
> This sounds as if there is no mapping from VAImageFormat
> to AVPixelFormat but I misunderstand this, no?

The fourcc (which is VAImageFormat.fourcc) is the thing which is actually used for the mapping.  The other components do not add anything to the interpretation, so they are ignored.  Indeed, the fourcc by itself is used in most other places (vaCreateSurfaces(), notably), while only vaCreateImage() takes the VAImageFormat argument.

>> So, I don't think this comment adds any value.
> 
> Do I understand correctly that one of the RGB formats
> work correctly on your (little-endian) system (and looks
> ugly if you replace it with anything else)?

I'm not sure what what you mean by this question?  The mapping works correctly for all of the supported RGB formats in the table, though I agree that if I modified the code to be incorrect then the output would be wrong.

For example, I can do screen capture in X with:

ffmpeg -y -vaapi_device /dev/dri/renderD128 -video_size 1920x1080 -framerate 30 -f x11grab -i :0 -vf 'hwupload,scale_vaapi=1920:1080:nv12' -c:v h264_vaapi out.mp4

which captures in bgr0, uploads it to the GPU, converts it to nv12 and encodes it as H.264 there.  Alternatively, I can add 'format=rgb0' at the start of the filter chain to convert and upload in a different RGB format, and that produces the correct output too.

Thanks,

- Mark