| Message ID | 20220404163533.707508-7-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] media: coda: set output buffer bytesused to appease v4l2-compliance | expand |
Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT, > to fix the following v4l2-compliance test failure: > > fail: v4l2-test-formats.cpp(1413): got error 22 when setting parms for buftype 1 > test VIDIOC_G/S_PARM: FAIL That one may be missing something though. As you don't implement performance target, you need to override the value somehow with the value you wrote into the bitstream no ? Otherwise we just ignore what userland sets silently ? I might not have got exactly how this case is supposed to be handled. Looking for feedback on what is proper behaviour for drivers that do not implement performance targets (resource reservation). > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/chips-media/coda-common.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c > index 33fcd8c7d72b..cd9ff2fa4147 100644 > --- a/drivers/media/platform/chips-media/coda-common.c > +++ b/drivers/media/platform/chips-media/coda-common.c > @@ -1421,9 +1421,6 @@ static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > struct coda_ctx *ctx = fh_to_ctx(fh); > struct v4l2_fract *tpf; > > - if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > - return -EINVAL; > - > a->parm.output.capability = V4L2_CAP_TIMEPERFRAME; > tpf = &a->parm.output.timeperframe; > coda_approximate_timeperframe(tpf);
Hi Nicolas, thank you for the review. You bring up a good point here, I think this part of the spec is not very easy to understand. On Di, 2022-04-05 at 10:22 -0400, Nicolas Dufresne wrote: > Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > > Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT, > > to fix the following v4l2-compliance test failure: > > > > fail: v4l2-test-formats.cpp(1413): got error 22 > > when setting parms for buftype 1 > > test VIDIOC_G/S_PARM: FAIL > > That one may be missing something though. As you don't implement performance > target, you need to override the value somehow with the value you wrote into the > bitstream no ? Otherwise we just ignore what userland sets silently ? You are right that we don't implement any performance targets. But the spec also says [1]: Changing the OUTPUT frame interval also sets the framerate that the encoder uses to encode the video. So setting the frame interval to 1/24 (or 24 frames per second) will produce a coded video stream that can be played back at that speed. The frame interval for the OUTPUT queue is just a hint, the application may provide raw frames at a different rate. It can be used by the driver to help schedule multiple encoders running in parallel. In the next step the CAPTURE frame interval can optionally be changed to a different value. This is useful for off-line encoding were the coded frame interval can be different from the rate at which raw frames are supplied. And Changing the CAPTURE frame interval sets the framerate for the coded video. It does not set the rate at which buffers arrive on the CAPTURE queue, that depends on how fast the encoder is and how fast raw frames are queued on the OUTPUT queue. [1] https://docs.kernel.org/userspace-api/media/v4l/dev-encoder.html?highlight=stateful%20encoder So far I'm only implementing the OUTPUT->CAPTURE rate passthrough part to make v4l2-compliance happy. Since the "frame interval for the OUTPUT queue is just a hint" and the spec only says that "the encoder may adjust it to match hardware requirements", I felt free to just ignore the raw frame interval part for now. My understanding is that the driver may limit this value to the achievable real time encoding speed (if it even knows). One thing I'm not doing according to spec right now is that calling S_PARM on CAPTURE will also change the OUTPUT interval, as the driver just stores them in the same variable. Also the driver does not set the V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL to signal it supports S_PARM on CAPTURE. My understanding is that the CAPTURE frame interval is the value that should be written to the bitstream and that should be used for the bitrate control algorithm. > I might not have got exactly how this case is supposed to be handled. > Looking for feedback on what is proper behaviour for drivers that do > not implement performance targets (resource reservation). Same here. I'd love to learn whether my understanding of the spec is correct or not. regards Philipp
Le mardi 05 avril 2022 à 17:03 +0200, Philipp Zabel a écrit : > Hi Nicolas, > > thank you for the review. You bring up a good point here, I think this > part of the spec is not very easy to understand. > > On Di, 2022-04-05 at 10:22 -0400, Nicolas Dufresne wrote: > > Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > > > Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT, > > > to fix the following v4l2-compliance test failure: > > > > > > fail: v4l2-test-formats.cpp(1413): got error 22 > > > when setting parms for buftype 1 > > > test VIDIOC_G/S_PARM: FAIL > > > > That one may be missing something though. As you don't implement performance > > target, you need to override the value somehow with the value you wrote into the > > bitstream no ? Otherwise we just ignore what userland sets silently ? > > You are right that we don't implement any performance targets. > But the spec also says [1]: > > Changing the OUTPUT frame interval also sets the framerate that the > encoder uses to encode the video. So setting the frame interval to > 1/24 (or 24 frames per second) will produce a coded video stream that > can be played back at that speed. The frame interval for the OUTPUT > queue is just a hint, the application may provide raw frames at a > different rate. It can be used by the driver to help schedule > multiple encoders running in parallel. > > In the next step the CAPTURE frame interval can optionally be changed > to a different value. This is useful for off-line encoding were the > coded frame interval can be different from the rate at which raw > frames are supplied. > > And > > Changing the CAPTURE frame interval sets the framerate for the coded > video. It does not set the rate at which buffers arrive on the > CAPTURE queue, that depends on how fast the encoder is and how fast > raw frames are queued on the OUTPUT queue. > > [1] https://docs.kernel.org/userspace-api/media/v4l/dev-encoder.html?highlight=stateful%20encoder > > So far I'm only implementing the OUTPUT->CAPTURE rate passthrough part > to make v4l2-compliance happy. > > Since the "frame interval for the OUTPUT queue is just a hint" and the > spec only says that "the encoder may adjust it to match hardware > requirements", I felt free to just ignore the raw frame interval part > for now. > My understanding is that the driver may limit this value to the > achievable real time encoding speed (if it even knows). > > One thing I'm not doing according to spec right now is that calling > S_PARM on CAPTURE will also change the OUTPUT interval, as the driver > just stores them in the same variable. > Also the driver does not set the V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL > to signal it supports S_PARM on CAPTURE. > My understanding is that the CAPTURE frame interval is the value that > should be written to the bitstream and that should be used for the > bitrate control algorithm. This is another very good point, if a driver does not set V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL, why can't it return ENOTTY for S_PARAM(CAPTURE) ? Is the test even correct? > > > I might not have got exactly how this case is supposed to be handled. > > Looking for feedback on what is proper behaviour for drivers that do > > not implement performance targets (resource reservation). > > Same here. I'd love to learn whether my understanding of the spec is > correct or not. > > regards > Philipp
diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c index 33fcd8c7d72b..cd9ff2fa4147 100644 --- a/drivers/media/platform/chips-media/coda-common.c +++ b/drivers/media/platform/chips-media/coda-common.c @@ -1421,9 +1421,6 @@ static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a) struct coda_ctx *ctx = fh_to_ctx(fh); struct v4l2_fract *tpf; - if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) - return -EINVAL; - a->parm.output.capability = V4L2_CAP_TIMEPERFRAME; tpf = &a->parm.output.timeperframe; coda_approximate_timeperframe(tpf);
Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT, to fix the following v4l2-compliance test failure: fail: v4l2-test-formats.cpp(1413): got error 22 when setting parms for buftype 1 test VIDIOC_G/S_PARM: FAIL Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/platform/chips-media/coda-common.c | 3 --- 1 file changed, 3 deletions(-)