| Message ID | 20210616204844.429936-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Vedant, For the subject, maybe "libcamera: Fix V4L2 pixel format mapping for XRGB32" ? On Thu, Jun 17, 2021 at 02:18:44AM +0530, Vedant Paranjape wrote: > According to DRM FourCCs and linux kernel docs V4L2_PIX_FMT_XRGB32 I find it odd that the DRM fourccs would document v4l2 pixel formats... > is BX24, but it was mapped to formats::XBGR8888, which is XB24. > > This patch fixes the mismatch of V4L2_PIX_FMT_XRGB32, i.e, matches it to > formats::BGRX8888. Furthermore, this patch adds V4L2_PIX_FMT_RBGX32 pixel "Additionally, add a mapping from V4L2_PIX_FMT_RGBX32 to formats::XBGR8888" ? Paul > format which is represented by XB24, and maps V4L2_PIX_FMT_RGBX32 > to formats::XBGR8888 > > Linux kernel pixel format docs: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-rgb.html#bits-per-component > DRM FourCC pixel format docs: https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L150 > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > --- > src/libcamera/v4l2_pixelformat.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > index 166d93cf..e86788ed 100644 > --- a/src/libcamera/v4l2_pixelformat.cpp > +++ b/src/libcamera/v4l2_pixelformat.cpp > @@ -51,7 +51,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > { V4L2PixelFormat(V4L2_PIX_FMT_RGB24), formats::BGR888 }, > { V4L2PixelFormat(V4L2_PIX_FMT_BGR24), formats::RGB888 }, > { V4L2PixelFormat(V4L2_PIX_FMT_XBGR32), formats::XRGB8888 }, > - { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::XBGR8888 }, > + { V4L2PixelFormat(V4L2_PIX_FMT_RGBX32), formats::XBGR8888 }, > + { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::BGRX8888 }, > { V4L2PixelFormat(V4L2_PIX_FMT_RGBA32), formats::ABGR8888 }, > { V4L2PixelFormat(V4L2_PIX_FMT_ABGR32), formats::ARGB8888 }, > { V4L2PixelFormat(V4L2_PIX_FMT_ARGB32), formats::BGRA8888 }, > -- > 2.25.1 >
On Thu, Jun 17, 2021 at 01:30:57PM +0900, paul.elder@ideasonboard.com wrote: > Hi Vedant, > > For the subject, maybe "libcamera: Fix V4L2 pixel format mapping for > XRGB32" ? > > On Thu, Jun 17, 2021 at 02:18:44AM +0530, Vedant Paranjape wrote: > > According to DRM FourCCs and linux kernel docs V4L2_PIX_FMT_XRGB32 > > I find it odd that the DRM fourccs would document v4l2 pixel formats... > > > is BX24, but it was mapped to formats::XBGR8888, which is XB24. The patch is correct, but the explanation isn't. There are 4CC values that refer to different formats in V4L2 and DRM. We can't rely on 'XR24' in DRM referring to the same format as 'XR24' in V4L2. > > This patch fixes the mismatch of V4L2_PIX_FMT_XRGB32, i.e, matches it to > > formats::BGRX8888. Furthermore, this patch adds V4L2_PIX_FMT_RBGX32 pixel > > "Additionally, add a mapping from V4L2_PIX_FMT_RGBX32 to formats::XBGR8888" ? > > Paul > > > format which is represented by XB24, and maps V4L2_PIX_FMT_RGBX32 > > to formats::XBGR8888 > > > > Linux kernel pixel format docs: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-rgb.html#bits-per-component > > DRM FourCC pixel format docs: https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L150 > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > src/libcamera/v4l2_pixelformat.cpp | 3 ++- You also need to update formats.cpp. > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > > index 166d93cf..e86788ed 100644 > > --- a/src/libcamera/v4l2_pixelformat.cpp > > +++ b/src/libcamera/v4l2_pixelformat.cpp > > @@ -51,7 +51,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > > { V4L2PixelFormat(V4L2_PIX_FMT_RGB24), formats::BGR888 }, > > { V4L2PixelFormat(V4L2_PIX_FMT_BGR24), formats::RGB888 }, > > { V4L2PixelFormat(V4L2_PIX_FMT_XBGR32), formats::XRGB8888 }, > > - { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::XBGR8888 }, > > + { V4L2PixelFormat(V4L2_PIX_FMT_RGBX32), formats::XBGR8888 }, > > + { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::BGRX8888 }, > > { V4L2PixelFormat(V4L2_PIX_FMT_RGBA32), formats::ABGR8888 }, > > { V4L2PixelFormat(V4L2_PIX_FMT_ABGR32), formats::ARGB8888 }, > > { V4L2PixelFormat(V4L2_PIX_FMT_ARGB32), formats::BGRA8888 },
Hi Laurent, On Thu, 17 Jun, 2021, 10:24 Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > On Thu, Jun 17, 2021 at 01:30:57PM +0900, paul.elder@ideasonboard.com > wrote: > > Hi Vedant, > > > > For the subject, maybe "libcamera: Fix V4L2 pixel format mapping for > > XRGB32" ? > > > > On Thu, Jun 17, 2021 at 02:18:44AM +0530, Vedant Paranjape wrote: > > > According to DRM FourCCs and linux kernel docs V4L2_PIX_FMT_XRGB32 > > > > I find it odd that the DRM fourccs would document v4l2 pixel formats... > > > > > is BX24, but it was mapped to formats::XBGR8888, which is XB24. > > The patch is correct, but the explanation isn't. There are 4CC values > that refer to different formats in V4L2 and DRM. We can't rely on 'XR24' > in DRM referring to the same format as 'XR24' in V4L2. > Then what is the common denominator between DRM and V4L2 pix format we can use to find a mapping between these ? Also can you please help me with a right commit message. It seems tricky in this case. > > This patch fixes the mismatch of V4L2_PIX_FMT_XRGB32, i.e, matches it to > > > formats::BGRX8888. Furthermore, this patch adds V4L2_PIX_FMT_RBGX32 > pixel > > > > "Additionally, add a mapping from V4L2_PIX_FMT_RGBX32 to > formats::XBGR8888" ? > > > > Paul > > > > > format which is represented by XB24, and maps V4L2_PIX_FMT_RGBX32 > > > to formats::XBGR8888 > > > > > > Linux kernel pixel format docs: > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-rgb.html#bits-per-component > > > DRM FourCC pixel format docs: > https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L150 > > > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > --- > > > src/libcamera/v4l2_pixelformat.cpp | 3 ++- > > You also need to update formats.cpp. > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp > b/src/libcamera/v4l2_pixelformat.cpp > > > index 166d93cf..e86788ed 100644 > > > --- a/src/libcamera/v4l2_pixelformat.cpp > > > +++ b/src/libcamera/v4l2_pixelformat.cpp > > > @@ -51,7 +51,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > > > { V4L2PixelFormat(V4L2_PIX_FMT_RGB24), formats::BGR888 }, > > > { V4L2PixelFormat(V4L2_PIX_FMT_BGR24), formats::RGB888 }, > > > { V4L2PixelFormat(V4L2_PIX_FMT_XBGR32), formats::XRGB8888 }, > > > - { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::XBGR8888 }, > > > + { V4L2PixelFormat(V4L2_PIX_FMT_RGBX32), formats::XBGR8888 }, > > > + { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::BGRX8888 }, > > > { V4L2PixelFormat(V4L2_PIX_FMT_RGBA32), formats::ABGR8888 }, > > > { V4L2PixelFormat(V4L2_PIX_FMT_ABGR32), formats::ARGB8888 }, > > > { V4L2PixelFormat(V4L2_PIX_FMT_ARGB32), formats::BGRA8888 }, > > -- > Regards, > > Laurent Pinchart >
Hi Vedant, On Thu, Jun 17, 2021 at 10:30:03AM +0530, Vedant Paranjape wrote: > On Thu, 17 Jun, 2021, 10:24 Laurent Pinchart wrote: > > On Thu, Jun 17, 2021 at 01:30:57PM +0900, paul.elder@ideasonboard.com wrote: > > > Hi Vedant, > > > > > > For the subject, maybe "libcamera: Fix V4L2 pixel format mapping for > > > XRGB32" ? > > > > > > On Thu, Jun 17, 2021 at 02:18:44AM +0530, Vedant Paranjape wrote: > > > > According to DRM FourCCs and linux kernel docs V4L2_PIX_FMT_XRGB32 > > > > > > I find it odd that the DRM fourccs would document v4l2 pixel formats... > > > > > > > is BX24, but it was mapped to formats::XBGR8888, which is XB24. > > > > The patch is correct, but the explanation isn't. There are 4CC values > > that refer to different formats in V4L2 and DRM. We can't rely on 'XR24' > > in DRM referring to the same format as 'XR24' in V4L2. > > > > Then what is the common denominator between DRM and V4L2 pix format we can > use to find a mapping between these ? Only the documentation I'm afraid. Documentation/userspace-api/media/v4l/pixfmt-rgb.rst for V4L2 (I highly encourage you to compile the kernel documentation and view the HTML output), and include/uapi/drm/drm_fourcc.h for DRM. Note that the former shows how the bytes are layed out in memory, while the latter shows the format of a work that is then stored in little endian, which results in bytes being swapped. > Also can you please help me with a right commit message. It seems tricky in > this case. I'll let Paul help :-) It would be useful if you split the patch in two, first a patch to fix the error, and then a patch to add the new format. > > > This patch fixes the mismatch of V4L2_PIX_FMT_XRGB32, i.e, matches it to > > > > formats::BGRX8888. Furthermore, this patch adds V4L2_PIX_FMT_RBGX32 pixel > > > > > > "Additionally, add a mapping from V4L2_PIX_FMT_RGBX32 to formats::XBGR8888" ? > > > > > > Paul > > > > > > > format which is represented by XB24, and maps V4L2_PIX_FMT_RGBX32 > > > > to formats::XBGR8888 > > > > > > > > Linux kernel pixel format docs: > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-rgb.html#bits-per-component > > > > DRM FourCC pixel format docs: > > https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L150 > > > > > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > > --- > > > > src/libcamera/v4l2_pixelformat.cpp | 3 ++- > > > > You also need to update formats.cpp. > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > > > > index 166d93cf..e86788ed 100644 > > > > --- a/src/libcamera/v4l2_pixelformat.cpp > > > > +++ b/src/libcamera/v4l2_pixelformat.cpp > > > > @@ -51,7 +51,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > > > > { V4L2PixelFormat(V4L2_PIX_FMT_RGB24), formats::BGR888 }, > > > > { V4L2PixelFormat(V4L2_PIX_FMT_BGR24), formats::RGB888 }, > > > > { V4L2PixelFormat(V4L2_PIX_FMT_XBGR32), formats::XRGB8888 }, > > > > - { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::XBGR8888 }, > > > > + { V4L2PixelFormat(V4L2_PIX_FMT_RGBX32), formats::XBGR8888 }, > > > > + { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::BGRX8888 }, > > > > { V4L2PixelFormat(V4L2_PIX_FMT_RGBA32), formats::ABGR8888 }, > > > > { V4L2PixelFormat(V4L2_PIX_FMT_ABGR32), formats::ARGB8888 }, > > > > { V4L2PixelFormat(V4L2_PIX_FMT_ARGB32), formats::BGRA8888 },
diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp index 166d93cf..e86788ed 100644 --- a/src/libcamera/v4l2_pixelformat.cpp +++ b/src/libcamera/v4l2_pixelformat.cpp @@ -51,7 +51,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ { V4L2PixelFormat(V4L2_PIX_FMT_RGB24), formats::BGR888 }, { V4L2PixelFormat(V4L2_PIX_FMT_BGR24), formats::RGB888 }, { V4L2PixelFormat(V4L2_PIX_FMT_XBGR32), formats::XRGB8888 }, - { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::XBGR8888 }, + { V4L2PixelFormat(V4L2_PIX_FMT_RGBX32), formats::XBGR8888 }, + { V4L2PixelFormat(V4L2_PIX_FMT_XRGB32), formats::BGRX8888 }, { V4L2PixelFormat(V4L2_PIX_FMT_RGBA32), formats::ABGR8888 }, { V4L2PixelFormat(V4L2_PIX_FMT_ABGR32), formats::ARGB8888 }, { V4L2PixelFormat(V4L2_PIX_FMT_ARGB32), formats::BGRA8888 },
According to DRM FourCCs and linux kernel docs V4L2_PIX_FMT_XRGB32 is BX24, but it was mapped to formats::XBGR8888, which is XB24. This patch fixes the mismatch of V4L2_PIX_FMT_XRGB32, i.e, matches it to formats::BGRX8888. Furthermore, this patch adds V4L2_PIX_FMT_RBGX32 pixel format which is represented by XB24, and maps V4L2_PIX_FMT_RGBX32 to formats::XBGR8888 Linux kernel pixel format docs: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-rgb.html#bits-per-component DRM FourCC pixel format docs: https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L150 Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> --- src/libcamera/v4l2_pixelformat.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)