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

[Carl Eugen Hoyos]

2016-10-05 21:55 GMT+02:00 Mark Thompson <sw at jkqxz.net>:
> 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.

Interesting.

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

Google finds some occurrences of VA_MSB_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.

What I meant is:
How can "LE" make sense for an 8bit planar format?
(And what does it tell us about the author?)
This of course assumes that "YV12" is planar, if it
isn't, I simply misunderstand the whole code.

>>> 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.

I start to understand that for 8bpc rgb, the mapping is relevant
(not the byte order) and is reflected in the fourcc.

>>> 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.

Thank you for confirming this.

Do you think vaapi's P010 should be mapped to FFmpeg's
P010LE instead of P010?

Carl Eugen