Re: FIMC/CAMIF V4L2 driver

Re: FIMC/CAMIF V4L2 driver

[Sylwester Nawrocki]

Hi Sebastian,

Cc: <linux-media@vger.kernel.org>

On 12/28/2012 10:02 AM, Sebastian Dröge wrote:
> Hi Sylwester,
> 
> Kamil Debski told me that you should be able to help me with any issues
> about the FIMC/CAMIF V4L2 driver. I'm currently using it on Exynos 4
> hardware and wrote a GStreamer plugin using it (and the MFC driver).
> 
> So far everything works great but I found a bug in the driver. When
> configuring the CAPTURE side to use the pixel format
> V4L2_PIX_FMT_YUV420M the strides that are reported are wrong.
> 
> I get them by setting a v4l2_format with VIDIOC_S_FMT and having the
> fmt.pix_mp.plane_fmt[X].bytesperline set to zero. The value set there
> after the ioctl is correct for the luma plane but has the same value for
> the chroma planes while it should be the half.
> By using a stride that is half the value I can get valid and usable
> output.
> 
> For V4L2_PIX_FMT_NV12MT and V4L2_PIX_FMT_NV12M these stride values are
> correct, so maybe a check for V4L2_PIX_FMT_YUV420M is missing somewhere
> to divide by two for the chroma planes.

Thank you for the bug report. And sorry for the delay..

The driver sets same bytesperline value for each plane, since I found
definition of this parameter very vague for planar formats, especially
the macro-block ones, e.g. [1]. So it's really a feature, not a bug ;)

Nevertheless, what the documentation [2] says is:

"\bytesperline\    Distance in bytes between the leftmost pixels in two
adjacent lines."
...

"When the image format is planar the bytesperline value applies to the
largest plane and is divided by the same factor as the width field for
any smaller planes. For example the Cb and Cr planes of a YUV 4:2:0 image
have half as many padding bytes following each line as the Y plane. To
avoid ambiguities drivers must return a bytesperline value rounded up to
a multiple of the scale factor."

Then, for V4L2_PIX_FMT_NV12M format bytesperline for both planes should be
same, since according to the format definition chroma and luma plane width
are same.

For V4L2_PIX_FMT_YUV420M the Cr and Cb plane is half the width and half
the height of the image (Y plane). I agree the bytesperline of the chroma
should be half of that of luma plane.

Please let me know if this patch helps:
http://patchwork.linuxtv.org/patch/16205

If it's OK then I'll submit it for v3.9 kernel.

[1] http://linuxtv.org/downloads/v4l-dvb-apis/re31.html
[2] http://linuxtv.org/downloads/v4l-dvb-apis/pixfmt.html#v4l2-pix-format

--

Regards,
Sylwester

Re: FIMC/CAMIF V4L2 driver

[Sebastian Dröge]

[-- Attachment #1: Type: text/plain, Size: 3077 bytes --]

On Fr, 2013-01-11 at 11:39 +0100, Sylwester Nawrocki wrote:
> Hi Sebastian,
> 
> Cc: <linux-media@vger.kernel.org>
> 
> On 12/28/2012 10:02 AM, Sebastian Dröge wrote:
> > Hi Sylwester,
> > 
> > Kamil Debski told me that you should be able to help me with any issues
> > about the FIMC/CAMIF V4L2 driver. I'm currently using it on Exynos 4
> > hardware and wrote a GStreamer plugin using it (and the MFC driver).
> > 
> > So far everything works great but I found a bug in the driver. When
> > configuring the CAPTURE side to use the pixel format
> > V4L2_PIX_FMT_YUV420M the strides that are reported are wrong.
> > 
> > I get them by setting a v4l2_format with VIDIOC_S_FMT and having the
> > fmt.pix_mp.plane_fmt[X].bytesperline set to zero. The value set there
> > after the ioctl is correct for the luma plane but has the same value for
> > the chroma planes while it should be the half.
> > By using a stride that is half the value I can get valid and usable
> > output.
> > 
> > For V4L2_PIX_FMT_NV12MT and V4L2_PIX_FMT_NV12M these stride values are
> > correct, so maybe a check for V4L2_PIX_FMT_YUV420M is missing somewhere
> > to divide by two for the chroma planes.
> 
> Thank you for the bug report. And sorry for the delay..
> 
> The driver sets same bytesperline value for each plane, since I found
> definition of this parameter very vague for planar formats, especially
> the macro-block ones, e.g. [1]. So it's really a feature, not a bug ;)
> 
> Nevertheless, what the documentation [2] says is:
> 
> "\bytesperline\    Distance in bytes between the leftmost pixels in two
> adjacent lines."
> ...
> 
> "When the image format is planar the bytesperline value applies to the
> largest plane and is divided by the same factor as the width field for
> any smaller planes. For example the Cb and Cr planes of a YUV 4:2:0 image
> have half as many padding bytes following each line as the Y plane. To
> avoid ambiguities drivers must return a bytesperline value rounded up to
> a multiple of the scale factor."
> 
> Then, for V4L2_PIX_FMT_NV12M format bytesperline for both planes should be
> same, since according to the format definition chroma and luma plane width
> are same.
> 
> For V4L2_PIX_FMT_YUV420M the Cr and Cb plane is half the width and half
> the height of the image (Y plane). I agree the bytesperline of the chroma
> should be half of that of luma plane.
> 
> Please let me know if this patch helps:
> http://patchwork.linuxtv.org/patch/16205

Thanks, especially for the long explanation of why it is like this :)

I can't test the patch right now but it should do almost the right
thing. IMHO for the chroma planes the bytesperline should be (width
+1)/2, otherwise you'll miss one chroma value per line for odd widths.


However I also noticed another bug. Currently S_FMT happily allows
V4L2_PIX_FMT_BGR32, V4L2_PIX_FMT_BGR24, V4L2_PIX_FMT_RGB24 and probably
others. But the output will be distorted and useless.
(V4L2_PIX_FMT_RGB32 works perfectly fine)


BR,
Sebastian

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: FIMC/CAMIF V4L2 driver

[Sylwester Nawrocki]

Hi,

On 01/11/2013 12:08 PM, Sebastian Dröge wrote:
> I can't test the patch right now but it should do almost the right
> thing. IMHO for the chroma planes the bytesperline should be (width
> +1)/2, otherwise you'll miss one chroma value per line for odd widths.

Odd widths are not allowed, the driver will adjust width to be multiple
of 16 pixels. However, you can adjust the usable area more precisely with
VIDIOC_S_CROP or VIDIOC_S_SELECTION ioctl. I still need to do some work to
define properly the selection ioctl on mem-to-mem devices in the V4L2
documentation.

> However I also noticed another bug. Currently S_FMT happily allows
> V4L2_PIX_FMT_BGR32, V4L2_PIX_FMT_BGR24, V4L2_PIX_FMT_RGB24 and probably
> others. But the output will be distorted and useless.
> (V4L2_PIX_FMT_RGB32 works perfectly fine)

This shouldn't really happen. Are you checking pixelformat after VIDIOC_S_FMT
call ? Isn't it adjusted to some valid and supported by the driver format ?

--

Regards,
Sylwester

Re: FIMC/CAMIF V4L2 driver

[Sebastian Dröge]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

On Fr, 2013-01-11 at 12:26 +0100, Sylwester Nawrocki wrote:
> Hi,
> 
> On 01/11/2013 12:08 PM, Sebastian Dröge wrote:
> > I can't test the patch right now but it should do almost the right
> > thing. IMHO for the chroma planes the bytesperline should be (width
> > +1)/2, otherwise you'll miss one chroma value per line for odd widths.
> 
> Odd widths are not allowed, the driver will adjust width to be multiple
> of 16 pixels. However, you can adjust the usable area more precisely with
> VIDIOC_S_CROP or VIDIOC_S_SELECTION ioctl. I still need to do some work to
> define properly the selection ioctl on mem-to-mem devices in the V4L2
> documentation.

Ok, thanks for the information :)

> > However I also noticed another bug. Currently S_FMT happily allows
> > V4L2_PIX_FMT_BGR32, V4L2_PIX_FMT_BGR24, V4L2_PIX_FMT_RGB24 and probably
> > others. But the output will be distorted and useless.
> > (V4L2_PIX_FMT_RGB32 works perfectly fine)
> 
> This shouldn't really happen. Are you checking pixelformat after VIDIOC_S_FMT
> call ? Isn't it adjusted to some valid and supported by the driver format ?

Good point, I didn't check it... but was assuming that the ioctl() would
instead fail. Thanks a lot!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]