[PATCH 1/3] [media] coda: fix coda_s_fmt_vid_out

[PATCH 1/3] [media] coda: fix coda_s_fmt_vid_out

[Philipp Zabel]

Set the context color space when s_fmt succeeded, not when it failed.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/media/platform/coda/coda-common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index dbd04ee..95d0b04 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -530,7 +530,9 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
 
 	ret = coda_s_fmt(ctx, f);
 	if (ret)
-		ctx->colorspace = f->fmt.pix.colorspace;
+		return ret;
+
+	ctx->colorspace = f->fmt.pix.colorspace;
 
 	return ret;
 }
-- 
2.0.1

[PATCH 2/3] [media] coda: fix coda_g_selection

[Philipp Zabel]

Crop targets are valid on the capture side and compose targets are valid
on the output side, not the other way around.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/media/platform/coda/coda-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 95d0b04..b542340 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -600,7 +600,7 @@ static int coda_g_selection(struct file *file, void *fh,
 		rsel = &r;
 		/* fallthrough */
 	case V4L2_SEL_TGT_CROP:
-		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
+		if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 			return -EINVAL;
 		break;
 	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
@@ -609,7 +609,7 @@ static int coda_g_selection(struct file *file, void *fh,
 		/* fallthrough */
 	case V4L2_SEL_TGT_COMPOSE:
 	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
-		if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
 			return -EINVAL;
 		break;
 	default:
-- 
2.0.1

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Hans Verkuil]

On 07/26/2014 04:34 PM, Philipp Zabel wrote:
> Crop targets are valid on the capture side and compose targets are valid
> on the output side, not the other way around.

Are you sure about this? Usually for m2m devices the capture side supports
compose (i.e. the result of the m2m operation can be composed into the capture
buffer) and the output side supports crop (i.e. the m2m operates on the cropped
part of the output buffer instead of on the full buffer), like the coda driver
does today.

As a result of that the old G/S_CROP API cannot be used with most m2m devices
since it does the opposite operation, which does not apply to m2m devices.

Regards,

	Hans

> 
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> ---
>  drivers/media/platform/coda/coda-common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 95d0b04..b542340 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -600,7 +600,7 @@ static int coda_g_selection(struct file *file, void *fh,
>  		rsel = &r;
>  		/* fallthrough */
>  	case V4L2_SEL_TGT_CROP:
> -		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +		if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  			return -EINVAL;
>  		break;
>  	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> @@ -609,7 +609,7 @@ static int coda_g_selection(struct file *file, void *fh,
>  		/* fallthrough */
>  	case V4L2_SEL_TGT_COMPOSE:
>  	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> -		if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>  			return -EINVAL;
>  		break;
>  	default:
> 

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Philipp Zabel]

On Sat, Jul 26, 2014 at 5:12 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 07/26/2014 04:34 PM, Philipp Zabel wrote:
>> Crop targets are valid on the capture side and compose targets are valid
>> on the output side, not the other way around.
>
> Are you sure about this? Usually for m2m devices the capture side supports
> compose (i.e. the result of the m2m operation can be composed into the capture
> buffer) and the output side supports crop (i.e. the m2m operates on the cropped
> part of the output buffer instead of on the full buffer), like the coda driver
> does today.

You are right, I haven't thought this through. Please ignore this patch.

> As a result of that the old G/S_CROP API cannot be used with most m2m devices
> since it does the opposite operation, which does not apply to m2m devices.

I have tried the GStreamer v4l2videodec element with the coda driver and
noticed that GStreamer calls VIDIOC_CROPCAP to obtain the pixel aspect
ratio. This always fails with -EINVAL because of this issue. Currently GStreamer
throws a warning if the return value is an error other than -ENOTTY.

regards
Philipp

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Hans Verkuil]

On 07/26/2014 06:37 PM, Philipp Zabel wrote:
> On Sat, Jul 26, 2014 at 5:12 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 07/26/2014 04:34 PM, Philipp Zabel wrote:
>>> Crop targets are valid on the capture side and compose targets are valid
>>> on the output side, not the other way around.
>>
>> Are you sure about this? Usually for m2m devices the capture side supports
>> compose (i.e. the result of the m2m operation can be composed into the capture
>> buffer) and the output side supports crop (i.e. the m2m operates on the cropped
>> part of the output buffer instead of on the full buffer), like the coda driver
>> does today.
> 
> You are right, I haven't thought this through. Please ignore this patch.
> 
>> As a result of that the old G/S_CROP API cannot be used with most m2m devices
>> since it does the opposite operation, which does not apply to m2m devices.
> 
> I have tried the GStreamer v4l2videodec element with the coda driver and
> noticed that GStreamer calls VIDIOC_CROPCAP to obtain the pixel aspect
> ratio. This always fails with -EINVAL because of this issue. Currently GStreamer
> throws a warning if the return value is an error other than -ENOTTY.

I never ever liked it that pixelaspect was part of cropcap since it is really
unrelated to cropping. Now that the compound control support is in it might
be time to create a pair of read-only controls that report the pixelaspect
ratio, one for video capture, one for video output. That would be a much
cleaner solution.

Regards,

	Hans

> 
> regards
> Philipp
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Nicolas Dufresne]

 
Le Samedi 26 Juillet 2014 12:37 EDT, Philipp Zabel <philipp.zabel@gmail.com> a écrit: 

> I have tried the GStreamer v4l2videodec element with the coda driver and
> noticed that GStreamer calls VIDIOC_CROPCAP to obtain the pixel aspect
> ratio. This always fails with -EINVAL because of this issue. Currently GStreamer
> throws a warning if the return value is an error other than -ENOTTY.


But for now, this seems like a fair thing to do. We currently assume that if your
driver is not implementing CROPCAP, then pixel aspect ratio at output will be
unchanged at capture. If there is an error though, it's not good sign, and we report
it. If that is wrong, let us know why and how to detect your driver error isn't a an error.

cheers,
Nicolas 
 
 
 

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Hans Verkuil]

On 07/27/2014 06:53 PM, Nicolas Dufresne wrote:
>  
> Le Samedi 26 Juillet 2014 12:37 EDT, Philipp Zabel <philipp.zabel@gmail.com> a écrit: 
> 
>> I have tried the GStreamer v4l2videodec element with the coda driver and
>> noticed that GStreamer calls VIDIOC_CROPCAP to obtain the pixel aspect
>> ratio. This always fails with -EINVAL because of this issue. Currently GStreamer
>> throws a warning if the return value is an error other than -ENOTTY.
> 
> But for now, this seems like a fair thing to do. We currently assume that if your
> driver is not implementing CROPCAP, then pixel aspect ratio at output will be
> unchanged at capture. If there is an error though, it's not good sign, and we report
> it. If that is wrong, let us know why and how to detect your driver error isn't a an error.

If cropcap returns -EINVAL then that means that the current input or output does
not support cropping (for input) or composing (for output). In that case the
pixel aspect ratio is undefined and you have no way to get hold of that information,
which is a bug in the V4L2 API.

In the case of an m2m device you can safely assume that whatever the pixel aspect
is of the image you give to the m2m device, it will still be the same pixel
aspect when you get it back. In fact, I would say that if an m2m device returns
cropcap information, then the pixel aspect ratio information is most likely not
applicable to the device and will typically be 1:1.

Pixel aspect ratio is only relevant if the video comes in or goes out to a physical
interface (sensor, video receiver/transmitter).

Regards,

	Hans

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Nicolas Dufresne]

Le dimanche 27 juillet 2014 à 20:21 +0200, Hans Verkuil a écrit :
> If cropcap returns -EINVAL then that means that the current input or
> output does
> not support cropping (for input) or composing (for output). In that case the
> pixel aspect ratio is undefined and you have no way to get hold of that information,
> which is a bug in the V4L2 API.
> 
> In the case of an m2m device you can safely assume that whatever the pixel aspect
> is of the image you give to the m2m device, it will still be the same pixel
> aspect when you get it back. In fact, I would say that if an m2m device returns
> cropcap information, then the pixel aspect ratio information is most likely not
> applicable to the device and will typically be 1:1.
> 
> Pixel aspect ratio is only relevant if the video comes in or goes out to a physical
> interface (sensor, video receiver/transmitter).

So far "not applicable" has been interpreted as not implemented /
ENOTTY. Can't CODA just do that and we can close this subject ?

Nicolas

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Hans Verkuil]

On 07/27/2014 11:32 PM, Nicolas Dufresne wrote:
> Le dimanche 27 juillet 2014 à 20:21 +0200, Hans Verkuil a écrit :
>> If cropcap returns -EINVAL then that means that the current input or
>> output does
>> not support cropping (for input) or composing (for output). In that case the
>> pixel aspect ratio is undefined and you have no way to get hold of that information,
>> which is a bug in the V4L2 API.
>>
>> In the case of an m2m device you can safely assume that whatever the pixel aspect
>> is of the image you give to the m2m device, it will still be the same pixel
>> aspect when you get it back. In fact, I would say that if an m2m device returns
>> cropcap information, then the pixel aspect ratio information is most likely not
>> applicable to the device and will typically be 1:1.
>>
>> Pixel aspect ratio is only relevant if the video comes in or goes out to a physical
>> interface (sensor, video receiver/transmitter).
> 
> So far "not applicable" has been interpreted as not implemented /
> ENOTTY. Can't CODA just do that and we can close this subject ?

Yes, that might be the best solution. Just call v4l2_disable_ioctl for CROPCAP and
G/S_CROP, since none of them apply to the coda driver.

Regards,

	Hans

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Philipp Zabel]

Am Montag, den 28.07.2014, 00:22 +0200 schrieb Hans Verkuil:
> On 07/27/2014 11:32 PM, Nicolas Dufresne wrote:
> > Le dimanche 27 juillet 2014 à 20:21 +0200, Hans Verkuil a écrit :
> >> If cropcap returns -EINVAL then that means that the current input or
> >> output does
> >> not support cropping (for input) or composing (for output). In that case the
> >> pixel aspect ratio is undefined and you have no way to get hold of that information,
> >> which is a bug in the V4L2 API.
> >>
> >> In the case of an m2m device you can safely assume that whatever the pixel aspect
> >> is of the image you give to the m2m device, it will still be the same pixel
> >> aspect when you get it back. In fact, I would say that if an m2m device returns
> >> cropcap information, then the pixel aspect ratio information is most likely not
> >> applicable to the device and will typically be 1:1.
> >>
> >> Pixel aspect ratio is only relevant if the video comes in or goes out to a physical
> >> interface (sensor, video receiver/transmitter).
> > 
> > So far "not applicable" has been interpreted as not implemented /
> > ENOTTY. Can't CODA just do that and we can close this subject ?
> 
> Yes, that might be the best solution. Just call v4l2_disable_ioctl for CROPCAP and
> G/S_CROP, since none of them apply to the coda driver.

Sounds like a plan, I'll do that.

thanks
Philipp

[PATCH 3/3] [media] coda: set capture frame size with output S_FMT

[Philipp Zabel]

This patch makes coda_s_fmt_vid_out propagate the output frame size
to the capture side.
The GStreamer v4l2videodec only ever calls S_FMT on the output side
and then expects G_FMT on the capture side to return a valid format.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/media/platform/coda/coda-common.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index b542340..789beb1 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -522,6 +522,7 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
 			      struct v4l2_format *f)
 {
 	struct coda_ctx *ctx = fh_to_ctx(priv);
+	struct v4l2_format f_cap;
 	int ret;
 
 	ret = coda_try_fmt_vid_out(file, priv, f);
@@ -534,7 +535,16 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
 
 	ctx->colorspace = f->fmt.pix.colorspace;
 
-	return ret;
+	f_cap.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	coda_g_fmt(file, priv, &f_cap);
+	f_cap.fmt.pix.width = f->fmt.pix.width;
+	f_cap.fmt.pix.height = f->fmt.pix.height;
+
+	ret = coda_try_fmt_vid_cap(file, priv, &f_cap);
+	if (ret)
+		return ret;
+
+	return coda_s_fmt(ctx, &f_cap);
 }
 
 static int coda_qbuf(struct file *file, void *priv,
-- 
2.0.1