[RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[Philipp Zabel]

The encoder needs to know the nominal framerate for the constant bitrate
control mechanism to work. Currently the only way to set the framerate is
by using VIDIOC_S_PARM on the output queue.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 39330a7..63eb510 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -803,6 +803,32 @@ static int coda_decoder_cmd(struct file *file, void *fh,
 	return 0;
 }
 
+static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
+{
+	struct coda_ctx *ctx = fh_to_ctx(fh);
+
+	a->parm.output.timeperframe.denominator = 1;
+	a->parm.output.timeperframe.numerator = ctx->params.framerate;
+
+	return 0;
+}
+
+static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
+{
+	struct coda_ctx *ctx = fh_to_ctx(fh);
+
+	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
+	    a->parm.output.timeperframe.numerator != 0) {
+		ctx->params.framerate = a->parm.output.timeperframe.denominator
+				      / a->parm.output.timeperframe.numerator;
+	}
+
+	a->parm.output.timeperframe.denominator = 1;
+	a->parm.output.timeperframe.numerator = ctx->params.framerate;
+
+	return 0;
+}
+
 static int coda_subscribe_event(struct v4l2_fh *fh,
 				const struct v4l2_event_subscription *sub)
 {
@@ -843,6 +869,9 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
 	.vidioc_try_decoder_cmd	= coda_try_decoder_cmd,
 	.vidioc_decoder_cmd	= coda_decoder_cmd,
 
+	.vidioc_g_parm		= coda_g_parm,
+	.vidioc_s_parm		= coda_s_parm,
+
 	.vidioc_subscribe_event = coda_subscribe_event,
 	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 };
-- 
2.1.4

Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[Hans Verkuil]

On 12/22/2014 05:00 PM, Philipp Zabel wrote:
> The encoder needs to know the nominal framerate for the constant bitrate
> control mechanism to work. Currently the only way to set the framerate is
> by using VIDIOC_S_PARM on the output queue.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/coda/coda-common.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 39330a7..63eb510 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -803,6 +803,32 @@ static int coda_decoder_cmd(struct file *file, void *fh,
>  	return 0;
>  }
>  
> +static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> +{
> +	struct coda_ctx *ctx = fh_to_ctx(fh);
> +

If a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT, then return -EINVAL. Ditto for s_parm.

> +	a->parm.output.timeperframe.denominator = 1;
> +	a->parm.output.timeperframe.numerator = ctx->params.framerate;
> +
> +	return 0;
> +}
> +
> +static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> +{
> +	struct coda_ctx *ctx = fh_to_ctx(fh);
> +
> +	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> +	    a->parm.output.timeperframe.numerator != 0) {
> +		ctx->params.framerate = a->parm.output.timeperframe.denominator
> +				      / a->parm.output.timeperframe.numerator;

Hmm, what happens if the denominator is 1 and the numerator is 2?
You probably want to clamp ctx->params.framerate to the range of allowed framerates.
And at least ensure a framerate > 0.

Also check with v4l2_compliance! You'd have caught at least the missing a->type check.

> +	}
> +
> +	a->parm.output.timeperframe.denominator = 1;
> +	a->parm.output.timeperframe.numerator = ctx->params.framerate;
> +
> +	return 0;
> +}
> +
>  static int coda_subscribe_event(struct v4l2_fh *fh,
>  				const struct v4l2_event_subscription *sub)
>  {
> @@ -843,6 +869,9 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
>  	.vidioc_try_decoder_cmd	= coda_try_decoder_cmd,
>  	.vidioc_decoder_cmd	= coda_decoder_cmd,
>  
> +	.vidioc_g_parm		= coda_g_parm,
> +	.vidioc_s_parm		= coda_s_parm,
> +
>  	.vidioc_subscribe_event = coda_subscribe_event,
>  	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>  };
> 

Regards,

	Hans

Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[Philipp Zabel]

Hi Hans,

thank you for the comments!

Am Montag, den 12.01.2015, 16:03 +0100 schrieb Hans Verkuil:
> On 12/22/2014 05:00 PM, Philipp Zabel wrote:
> > The encoder needs to know the nominal framerate for the constant bitrate
> > control mechanism to work. Currently the only way to set the framerate is
> > by using VIDIOC_S_PARM on the output queue.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/media/platform/coda/coda-common.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> > index 39330a7..63eb510 100644
> > --- a/drivers/media/platform/coda/coda-common.c
> > +++ b/drivers/media/platform/coda/coda-common.c
> > @@ -803,6 +803,32 @@ static int coda_decoder_cmd(struct file *file, void *fh,
> >  	return 0;
> >  }
> >  
> > +static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> > +{
> > +	struct coda_ctx *ctx = fh_to_ctx(fh);
> > +
> 
> If a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT, then return -EINVAL.

If the decoder can retrieve the framerate from the stream, wouldn't it
make sense to allow G_PARM for a->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ?

>  Ditto for s_parm.

Will do.

> > +	a->parm.output.timeperframe.denominator = 1;
> > +	a->parm.output.timeperframe.numerator = ctx->params.framerate;
> > +
> > +	return 0;
> > +}
> > +
> > +static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> > +{
> > +	struct coda_ctx *ctx = fh_to_ctx(fh);
> > +
> > +	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> > +	    a->parm.output.timeperframe.numerator != 0) {
> > +		ctx->params.framerate = a->parm.output.timeperframe.denominator
> > +				      / a->parm.output.timeperframe.numerator;
> 
> Hmm, what happens if the denominator is 1 and the numerator is 2?
> You probably want to clamp ctx->params.framerate to the range of allowed framerates.
> And at least ensure a framerate > 0.
> 
> Also check with v4l2_compliance! You'd have caught at least the missing a->type check.

Oh dear, I need to improve my v4l-utils update habits. I'll fix this
patch and resend it.

regards
Philipp

Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[Hans Verkuil]

On 01/12/2015 04:35 PM, Philipp Zabel wrote:
> Hi Hans,
> 
> thank you for the comments!
> 
> Am Montag, den 12.01.2015, 16:03 +0100 schrieb Hans Verkuil:
>> On 12/22/2014 05:00 PM, Philipp Zabel wrote:
>>> The encoder needs to know the nominal framerate for the constant bitrate
>>> control mechanism to work. Currently the only way to set the framerate is
>>> by using VIDIOC_S_PARM on the output queue.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  drivers/media/platform/coda/coda-common.c | 29 +++++++++++++++++++++++++++++
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
>>> index 39330a7..63eb510 100644
>>> --- a/drivers/media/platform/coda/coda-common.c
>>> +++ b/drivers/media/platform/coda/coda-common.c
>>> @@ -803,6 +803,32 @@ static int coda_decoder_cmd(struct file *file, void *fh,
>>>  	return 0;
>>>  }
>>>  
>>> +static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>>> +{
>>> +	struct coda_ctx *ctx = fh_to_ctx(fh);
>>> +
>>
>> If a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT, then return -EINVAL.
> 
> If the decoder can retrieve the framerate from the stream, wouldn't it
> make sense to allow G_PARM for a->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ?

Certainly. But that wasn't in the patch :-)

Regards,

	Hans

> 
>>  Ditto for s_parm.
> 
> Will do.
> 
>>> +	a->parm.output.timeperframe.denominator = 1;
>>> +	a->parm.output.timeperframe.numerator = ctx->params.framerate;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>>> +{
>>> +	struct coda_ctx *ctx = fh_to_ctx(fh);
>>> +
>>> +	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
>>> +	    a->parm.output.timeperframe.numerator != 0) {
>>> +		ctx->params.framerate = a->parm.output.timeperframe.denominator
>>> +				      / a->parm.output.timeperframe.numerator;
>>
>> Hmm, what happens if the denominator is 1 and the numerator is 2?
>> You probably want to clamp ctx->params.framerate to the range of allowed framerates.
>> And at least ensure a framerate > 0.
>>
>> Also check with v4l2_compliance! You'd have caught at least the missing a->type check.
> 
> Oh dear, I need to improve my v4l-utils update habits. I'll fix this
> patch and resend it.
> 
> regards
> Philipp
> 

Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[Nicolas Dufresne]

Le 2015-01-15 11:23, Frédéric Sureau a écrit :
> Maybe a->parm.output.capability should be set to 
> |V4L2_CAP_TIMEPERFRAME| here.
> I think it is required by GStreamer V4L2 plugin.
Looking at this, I think output device is indeed the right place to set 
this, and the capability should indeed be updated. Now for the GStreamer 
side, it will need patching. At the moment, this cap is only checked for 
capture device. It's not a major change to enabled that for output 
device with that capability.

cheers,
Nicolas

Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[Frédéric Sureau]

Hi Philipp,

Le 22/12/2014 17:00, Philipp Zabel a écrit :
> The encoder needs to know the nominal framerate for the constant bitrate
> control mechanism to work. Currently the only way to set the framerate is
> by using VIDIOC_S_PARM on the output queue.
>
> Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
> ---
>   drivers/media/platform/coda/coda-common.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 39330a7..63eb510 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -803,6 +803,32 @@ static int coda_decoder_cmd(struct file *file, void *fh,
>   	return 0;
>   }
>   
> +static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> +{
> +	struct coda_ctx *ctx = fh_to_ctx(fh);
> +
> +	a->parm.output.timeperframe.denominator = 1;
> +	a->parm.output.timeperframe.numerator = ctx->params.framerate;
> +
Maybe a->parm.output.capability should be set to V4L2_CAP_TIMEPERFRAME here.
I think it is required by GStreamer V4L2 plugin.
> +	return 0;
> +}
> +
> +static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> +{
> +	struct coda_ctx *ctx = fh_to_ctx(fh);
> +
> +	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> +	    a->parm.output.timeperframe.numerator != 0) {
> +		ctx->params.framerate = a->parm.output.timeperframe.denominator
> +				      / a->parm.output.timeperframe.numerator;
> +	}
> +
> +	a->parm.output.timeperframe.denominator = 1;
> +	a->parm.output.timeperframe.numerator = ctx->params.framerate;
> +
> +	return 0;
> +}
> +
>   static int coda_subscribe_event(struct v4l2_fh *fh,
>   				const struct v4l2_event_subscription *sub)
>   {
> @@ -843,6 +869,9 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
>   	.vidioc_try_decoder_cmd	= coda_try_decoder_cmd,
>   	.vidioc_decoder_cmd	= coda_decoder_cmd,
>   
> +	.vidioc_g_parm		= coda_g_parm,
> +	.vidioc_s_parm		= coda_s_parm,
> +
>   	.vidioc_subscribe_event = coda_subscribe_event,
>   	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>   };
Thanks for the patch!
Fred