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

2015-01-15 Thread 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 Zabelp.za...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-01-15 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-01-12 Thread 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.za...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-01-12 Thread 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.za...@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
 

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

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

2015-01-12 Thread 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.za...@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

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

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

2014-12-22 Thread 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.za...@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

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