| Message ID | 201610051406.47000.cehoyos@ag.or.at |
|---|---|
| State | Rejected |
| Headers | show |
On 05/10/16 13:06, Carl Eugen Hoyos wrote: > --- > Hi! > > I cannot test here but afaict, the current implementation of > vaapi_pix_fmt_from_fourcc() can't be correct. > > Please comment, Carl Eugen > > libavutil/hwcontext_vaapi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c > index 92fa235..3319839 100644 > --- a/libavutil/hwcontext_vaapi.c > +++ b/libavutil/hwcontext_vaapi.c > @@ -92,6 +92,7 @@ typedef struct VAAPISurfaceMap { > } > // The map fourcc <-> pix_fmt isn't bijective because of the annoying U/V > // plane swap cases. The frame handling below tries to hide these. > +// FIXME: Take VAImageFormat->byte_order into account > static struct { > unsigned int fourcc; > unsigned int rt_format; > -- > 1.7.10.4 Hi Carl, 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). 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: * <https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/va/image.c#n41> * <https://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c#n246> 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. So, I don't think this comment adds any value. Thanks, - Mark PS: Testing with mesa (or any other driver) on a big-endian machine would be very welcome, if someone has the hardware to do it.
2016-10-05 16:39 GMT+02:00 Mark Thompson <sw@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? > <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... > 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? > 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)? Carl Eugen
On 05/10/16 19:02, Carl Eugen Hoyos wrote: > 2016-10-05 16:39 GMT+02:00 Mark Thompson <sw@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
2016-10-05 21:55 GMT+02:00 Mark Thompson <sw@jkqxz.net>: > On 05/10/16 19:02, Carl Eugen Hoyos wrote: >> 2016-10-05 16:39 GMT+02:00 Mark Thompson <sw@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
On 06/10/16 09:05, Carl Eugen Hoyos wrote: > 2016-10-05 21:55 GMT+02:00 Mark Thompson <sw@jkqxz.net>: > > What I meant is: > How can "LE" make sense for an 8bit planar format? It does not. So, another reason to ignore that field. > (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. Yes, YV12 is planar: it is ffmpeg YUV420P with the two chroma planes swapped. >> 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? P010 is defined as format with a 16-bit unit size, so the native format on a BE system should be P010BE. I admit that confusion is likely, though, given that the actual hardware may be a graphics card which normally works with an LE host. We may need to look somewhere else for the answer at that point (possibly the byte_order field, assuming the drivers manage to fill it correctly in such cases). I would prefer to only consider this problem once we have some working system to test on. On the other hand, if you wish to submit a patch changing it now I would not mind - it would have no effect on current use because the one system with working 10-bit support is LE-only. Thanks, - Mark
2016-10-06 11:34 GMT+02:00 Mark Thompson <sw@jkqxz.net>: >> Do you think vaapi's P010 should be mapped to FFmpeg's >> P010LE instead of P010? > I would prefer to only consider this problem once we have > some working system to test on. No objections. Thank you, Carl Eugen
On Thu, 6 Oct 2016 10:34:35 +0100 Mark Thompson <sw@jkqxz.net> wrote: > On 06/10/16 09:05, Carl Eugen Hoyos wrote: > > 2016-10-05 21:55 GMT+02:00 Mark Thompson <sw@jkqxz.net>: > > > > What I meant is: > > How can "LE" make sense for an 8bit planar format? > > It does not. So, another reason to ignore that field. > > > (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. > > Yes, YV12 is planar: it is ffmpeg YUV420P with the two chroma planes swapped. > > >> 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? > > P010 is defined as format with a 16-bit unit size, so the native format on a BE > system should be P010BE. I admit that confusion is likely, though, given that > the actual hardware may be a graphics card which normally works with an LE host. > We may need to look somewhere else for the answer at that point (possibly the > byte_order field, assuming the drivers manage to fill it correctly in such cases). > > I would prefer to only consider this problem once we have some working system to > test on. On the other hand, if you wish to submit a patch changing it now I > would not mind - it would have no effect on current use because the one system > with working 10-bit support is LE-only. Is it even theoretically possible to have hardware using vaapi on big endian systems? vaapi is for modern Intel GPUs, which AFAIK are all little endian.
diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c index 92fa235..3319839 100644 --- a/libavutil/hwcontext_vaapi.c +++ b/libavutil/hwcontext_vaapi.c @@ -92,6 +92,7 @@ typedef struct VAAPISurfaceMap { } // The map fourcc <-> pix_fmt isn't bijective because of the annoying U/V // plane swap cases. The frame handling below tries to hide these. +// FIXME: Take VAImageFormat->byte_order into account static struct { unsigned int fourcc; unsigned int rt_format;
Hi! I cannot test here but afaict, the current implementation of vaapi_pix_fmt_from_fourcc() can't be correct. Please comment, Carl Eugen From 52b0af32b965c2fce23afe8b57289e497f17f011 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <cehoyos@ag.or.at> Date: Wed, 5 Oct 2016 14:04:25 +0200 Subject: [PATCH] lavu/hwcontext_vaapi: Add a fixme for the missing byte_order check. --- libavutil/hwcontext_vaapi.c | 1 + 1 file changed, 1 insertion(+)