* [PATCH] media: imx: utils: Default colorspace to SRGB
@ 2020-03-25 17:34 Steve Longerbeam
2020-04-16 10:52 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Steve Longerbeam @ 2020-03-25 17:34 UTC (permalink / raw)
To: linux-media
Cc: Hans Verkuil, Laurent Pinchart, Rui Miguel Silva, Philipp Zabel,
Steve Longerbeam
The function init_mbus_colorimetry() is used to initialize the imx
subdevice mbus colorimetry to some sane defaults when the subdevice is
registered. Currently it guesses at a colorspace based on the passed
mbus pixel format. If the format is RGB, it chooses colorspace
V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses
V4L2_COLORSPACE_SMPTE170M.
While that might be a good guess, it's not necessarily true that a RGB
pixel format encoding uses a SRGB colorspace, or that a YUV encoding
uses a SMPTE170M colorspace. Instead of making this dubious guess,
just default the colorspace to SRGB.
Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/staging/media/imx/imx-media-utils.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index fae981698c49..8344675bfd05 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = {
static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
const struct imx_media_pixfmt *fmt)
{
- mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ?
- V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M;
+ mbus->colorspace = V4L2_COLORSPACE_SRGB;
mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace);
mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace);
mbus->quantization =
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] media: imx: utils: Default colorspace to SRGB
2020-03-25 17:34 [PATCH] media: imx: utils: Default colorspace to SRGB Steve Longerbeam
@ 2020-04-16 10:52 ` Hans Verkuil
2020-04-16 17:10 ` Steve Longerbeam
0 siblings, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2020-04-16 10:52 UTC (permalink / raw)
To: Steve Longerbeam, linux-media
Cc: Laurent Pinchart, Rui Miguel Silva, Philipp Zabel
Hi Steve,
On 25/03/2020 18:34, Steve Longerbeam wrote:
> The function init_mbus_colorimetry() is used to initialize the imx
> subdevice mbus colorimetry to some sane defaults when the subdevice is
> registered. Currently it guesses at a colorspace based on the passed
> mbus pixel format. If the format is RGB, it chooses colorspace
> V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses
> V4L2_COLORSPACE_SMPTE170M.
>
> While that might be a good guess, it's not necessarily true that a RGB
> pixel format encoding uses a SRGB colorspace, or that a YUV encoding
> uses a SMPTE170M colorspace. Instead of making this dubious guess,
> just default the colorspace to SRGB.
>
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
> drivers/staging/media/imx/imx-media-utils.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index fae981698c49..8344675bfd05 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = {
> static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
> const struct imx_media_pixfmt *fmt)
> {
> - mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ?
> - V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M;
> + mbus->colorspace = V4L2_COLORSPACE_SRGB;
> mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace);
> mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace);
> mbus->quantization =
The quantization is probably wrong as well since it checks fmt->cs.
The first argument should just be 'true'.
>
In any case, this patch no longer applies after the imx utils patch series.
Regards,
Hans
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] media: imx: utils: Default colorspace to SRGB
2020-04-16 10:52 ` Hans Verkuil
@ 2020-04-16 17:10 ` Steve Longerbeam
2020-04-17 7:13 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Steve Longerbeam @ 2020-04-16 17:10 UTC (permalink / raw)
To: Hans Verkuil, linux-media
Cc: Laurent Pinchart, Rui Miguel Silva, Philipp Zabel
Hi Hans,
On 4/16/20 3:52 AM, Hans Verkuil wrote:
> Hi Steve,
>
> On 25/03/2020 18:34, Steve Longerbeam wrote:
>> The function init_mbus_colorimetry() is used to initialize the imx
>> subdevice mbus colorimetry to some sane defaults when the subdevice is
>> registered. Currently it guesses at a colorspace based on the passed
>> mbus pixel format. If the format is RGB, it chooses colorspace
>> V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses
>> V4L2_COLORSPACE_SMPTE170M.
>>
>> While that might be a good guess, it's not necessarily true that a RGB
>> pixel format encoding uses a SRGB colorspace, or that a YUV encoding
>> uses a SMPTE170M colorspace. Instead of making this dubious guess,
>> just default the colorspace to SRGB.
>>
>> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>> drivers/staging/media/imx/imx-media-utils.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>> index fae981698c49..8344675bfd05 100644
>> --- a/drivers/staging/media/imx/imx-media-utils.c
>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>> @@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = {
>> static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
>> const struct imx_media_pixfmt *fmt)
>> {
>> - mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ?
>> - V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M;
>> + mbus->colorspace = V4L2_COLORSPACE_SRGB;
>> mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace);
>> mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace);
>> mbus->quantization =
> The quantization is probably wrong as well since it checks fmt->cs.
> The first argument should just be 'true'.
I don't think so. The default quantization depends in part on whether
the pixel format is RGB or YUV, which is the reason for passing boolean
(fmt->cs == IPUV3_COLORSPACE_RGB) as the first argument.
I realize "fmt->cs" is a misnomer, but it is borrowing from earlier
misnomers, i.e. the 'enum ipu_color_space' enumerations. Fixing those
names would touch lots of imx driver code.
>
> In any case, this patch no longer applies after the imx utils patch series.
Ok, I'll re-submit after the utils patch series is merged.
Steve
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] media: imx: utils: Default colorspace to SRGB
2020-04-16 17:10 ` Steve Longerbeam
@ 2020-04-17 7:13 ` Hans Verkuil
0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2020-04-17 7:13 UTC (permalink / raw)
To: Steve Longerbeam, linux-media
Cc: Laurent Pinchart, Rui Miguel Silva, Philipp Zabel
On 16/04/2020 19:10, Steve Longerbeam wrote:
> Hi Hans,
>
> On 4/16/20 3:52 AM, Hans Verkuil wrote:
>> Hi Steve,
>>
>> On 25/03/2020 18:34, Steve Longerbeam wrote:
>>> The function init_mbus_colorimetry() is used to initialize the imx
>>> subdevice mbus colorimetry to some sane defaults when the subdevice is
>>> registered. Currently it guesses at a colorspace based on the passed
>>> mbus pixel format. If the format is RGB, it chooses colorspace
>>> V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses
>>> V4L2_COLORSPACE_SMPTE170M.
>>>
>>> While that might be a good guess, it's not necessarily true that a RGB
>>> pixel format encoding uses a SRGB colorspace, or that a YUV encoding
>>> uses a SMPTE170M colorspace. Instead of making this dubious guess,
>>> just default the colorspace to SRGB.
>>>
>>> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>>> ---
>>> drivers/staging/media/imx/imx-media-utils.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>>> index fae981698c49..8344675bfd05 100644
>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>> @@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = {
>>> static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
>>> const struct imx_media_pixfmt *fmt)
>>> {
>>> - mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ?
>>> - V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M;
>>> + mbus->colorspace = V4L2_COLORSPACE_SRGB;
>>> mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace);
>>> mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace);
>>> mbus->quantization =
>> The quantization is probably wrong as well since it checks fmt->cs.
>> The first argument should just be 'true'.
>
> I don't think so. The default quantization depends in part on whether
> the pixel format is RGB or YUV, which is the reason for passing boolean
> (fmt->cs == IPUV3_COLORSPACE_RGB) as the first argument.
Ah yes, that's true. Sorry for the noise.
Regards,
Hans
>
> I realize "fmt->cs" is a misnomer, but it is borrowing from earlier
> misnomers, i.e. the 'enum ipu_color_space' enumerations. Fixing those
> names would touch lots of imx driver code.
>
>
>>
>> In any case, this patch no longer applies after the imx utils patch series.
>
> Ok, I'll re-submit after the utils patch series is merged.
>
> Steve
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-17 7:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 17:34 [PATCH] media: imx: utils: Default colorspace to SRGB Steve Longerbeam
2020-04-16 10:52 ` Hans Verkuil
2020-04-16 17:10 ` Steve Longerbeam
2020-04-17 7:13 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).