On 4/10/19 6:11 PM, Nicolas Dufresne wrote: > Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit : >> On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote: >> [...] >>>> @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh, >> [...] >>> Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec. >>> I'd remove it altogether. > > It does make sense, since framerate is the only information that can be > used to produce a specific bitrate. If you don't enumerate the rates, > then you may endup with a miss-match of what userspace wants, which > will result in a different rate then what the user-space anticipated. > That being said, I expect these intervals to be really wide. Venus HW > uses a Q16 internally, which is precise enough that we could just > ignore the interval. So the problem is that for an encoder where you desires a specific constant bitrate, the encoder also needs to know the framerate. And the driver then would have to support ENUM_FRAMEINTERVALS and the S_PARM ioctl so userspace can set the framerate. For decoders this would not make any sense AFAIKS, so this is encoder specific. Do I understand this correctly? What should the default framerate be? 24 or 29.97 fps? This should be documented in the stateful encoder spec. I never realized that this would be needed... Philipp, based on this information I would say that ENUM_FRAMEINTERVALS can stay in the coda drivers, but for encoders only. Regards, Hans > >> >> It returns the range supported by the frame rate registers that can be >> set for constant bitrate encoding. I think the idea was to let the >> GStreamer v4l2 elements know about possible frame rate range. >> I think I should be able to remove it without any negative effects. > > As long as we can still set the framerate. > >> >> regards >> Philipp
