Re: [PATCH 7/7] media: coda: enable capture S_PARM for stateful encoder

[Philipp Zabel <p.zabel@pengutronix.de>]

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