Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

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

On Wed, 2019-04-10 at 18:24 +0200, Hans Verkuil wrote:
> 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.

Ah right, that was it. Encoding works fine without ENUM_FRAMEINTERVALS,
as coda has no meaningful frame rate limitations. I had implemented
S_PARM since it is required for CBR encoding, and v4l2-compliance then
complained about ENUM_FRAMEINTERVALS missing:

  cde29ef313de ("[media] coda: Use S_PARM to set nominal framerate for h.264 encoder")
  07b6080d4e6d ("media: coda: implement ENUM_FRAMEINTERVALS")

> For decoders this would not make any sense AFAIKS, so this is encoder
> specific.

Hmm, not necesarily. I hadn't thought about that before, but if the
decoder supports frame skipping this could be used to advertise
available reductions in frame rate.

> Do I understand this correctly?
> 
> What should the default framerate be? 24 or 29.97 fps?

Driver specific? Max nominal real time capable frame rate? 30 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.

Ok.

regards
Philipp