On 4/11/19 10:22 AM, Philipp Zabel wrote:
> 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")
And I think that's right. If you advertise framerate support, then
userspace needs to know about the capabilities.
I do think ENUM_FRAMEINTERVALS should return some sane values. Right now
coda advertises 1/65535 fps to 65535 fps (or thereabout).
I would probably return 1 fps to 200 fps (200 Hz is the highest framerate
defined by CTA-861-G).
This can also eventually be a helper function in v4l2-mem2mem.c since I expect
this to be the same for all (?) encoders.
>
>> 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.
That assumes that you want to use S_PARM for that as well. And does the decoder
provide the framerate encoded in the bitstream to the driver? Using S_PARM for
this only works if the driver can know the encoded framerate.
Anyway, this is probably something to discuss once a driver supports frame
skipping for the decoder.
>
>> 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
>
Regards,
Hans
