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
