| Message ID | 20190408123256.22868-7-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] media: coda: set codec earlier | expand |
On 4/8/19 2:32 PM, Philipp Zabel wrote: > Let VIDIOC_ENUM_FRAMEINTERVALS return -EINVAL if userspace queries > frame intervals for unsupported frame sizes. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/coda/coda-common.c | 33 ++++++++++++++++++----- > 1 file changed, 27 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c > index 943f003c26c4..2966eb1c4d2d 100644 > --- a/drivers/media/platform/coda/coda-common.c > +++ b/drivers/media/platform/coda/coda-common.c > @@ -1117,7 +1117,8 @@ static int coda_enum_frameintervals(struct file *file, void *fh, > struct v4l2_frmivalenum *f) > { > struct coda_ctx *ctx = fh_to_ctx(fh); > - int i; > + struct coda_q_data *q_data; > + const struct coda_codec *codec; > > if (f->index) > return -EINVAL; > @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh, > if (!ctx->vdoa && f->pixel_format == V4L2_PIX_FMT_YUYV) > return -EINVAL; > > - for (i = 0; i < CODA_MAX_FORMATS; i++) { > - if (f->pixel_format == ctx->cvd->src_formats[i] || > - f->pixel_format == ctx->cvd->dst_formats[i]) > - break; > + if (ctx->inst_type == CODA_INST_ENCODER) { > + if (coda_format_normalize_yuv(f->pixel_format) == > + V4L2_PIX_FMT_YUV420) { > + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > + codec = coda_find_codec(ctx->dev, f->pixel_format, > + q_data->fourcc); > + } else { > + codec = coda_find_codec(ctx->dev, V4L2_PIX_FMT_YUV420, > + f->pixel_format); > + } > + } else { > + if (coda_format_normalize_yuv(f->pixel_format) == > + V4L2_PIX_FMT_YUV420) { > + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); > + codec = coda_find_codec(ctx->dev, q_data->fourcc, > + f->pixel_format); > + } else { > + codec = coda_find_codec(ctx->dev, f->pixel_format, > + V4L2_PIX_FMT_YUV420); > + } > } > - if (i == CODA_MAX_FORMATS) > + if (!codec) > + return -EINVAL; > + > + if (f->width < MIN_W || f->width > codec->max_w || > + f->height < MIN_H || f->height > codec->max_h) > return -EINVAL; > > f->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; > Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec. I'd remove it altogether. Regards, Hans
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 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. regards Philipp
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. > > 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
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
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
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
Hi, On 11/04/2019 11:18, Hans Verkuil wrote: > 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. > While an i.MX wouldn't be my 1st choice for encoding a high framerate video, it could probably be done. This is a value for the bitrate control algo, afaics, not a statement of real time performance. If an arbitrary limit is imposed then userspace would just need to work around it by adjusting the target bitrate for corner cases like this. Regards, Ian >> >>> 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 >
On Thu, 2019-04-11 at 12:18 +0200, Hans Verkuil wrote: > 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). Those are the limits imposed by the frame rate registers. > I would probably return 1 fps to 200 fps (200 Hz is the highest framerate > defined by CTA-861-G). Why? We can certainly encode more than 200 fps in real time if only the frame size is small enough, and for offline transcoding the nominal frame rate can be anything the coded format supports. Smaller than 1 fps might be useful for time lapse videos as well. > This can also eventually be a helper function in v4l2-mem2mem.c since I expect > this to be the same for all (?) encoders. For example h.264 has an optional 32-bit numerator and denominator (num_units_in_tick and time_scale in the Video Usability Information). I don't see any reason to limit this artificially in the driver. > > 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? That is a difficult question. Yes, depending on CODA hardware variant, codec, and actual bitstream (where this information is optional), the firmware may or may not report the frame rate. Maybe it is fundamentally not a good idea to try to control frame skipping via the nominal frame rate instead of directly. > 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. Right. Since the coda driver does not support frame skipping yet, I'll return -ENOTTY for decoder ENUM_FRAMEINTERVALS. regards Philipp
Le jeudi 11 avril 2019 à 14:00 +0200, Philipp Zabel a écrit : > On Thu, 2019-04-11 at 12:18 +0200, Hans Verkuil wrote: > > 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). > > Those are the limits imposed by the frame rate registers. > > > I would probably return 1 fps to 200 fps (200 Hz is the highest framerate > > defined by CTA-861-G). > > Why? We can certainly encode more than 200 fps in real time if only the > frame size is small enough, and for offline transcoding the nominal > frame rate can be anything the coded format supports. > Smaller than 1 fps might be useful for time lapse videos as well. > > > This can also eventually be a helper function in v4l2-mem2mem.c since I expect > > this to be the same for all (?) encoders. > > For example h.264 has an optional 32-bit numerator and denominator > (num_units_in_tick and time_scale in the Video Usability Information). > I don't see any reason to limit this artificially in the driver. > > > > 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? > > That is a difficult question. Yes, depending on CODA hardware variant, > codec, and actual bitstream (where this information is optional), the > firmware may or may not report the frame rate. Maybe it is fundamentally > not a good idea to try to control frame skipping via the nominal frame > rate instead of directly. The Xilinx VCU decoder will allocate a different amount of cores per session depending on the framerate we give it. This is how they manage to handle more stream for lower rate/resolution etc. And it makes the S_PARM call mandatory, since faking one will always lead to some streams not to work properly, or less streams being handled in parallel. I personally don't see why we should in anyway be restrictive to the information we are allowed to pass to a driver. Specially that is does not matter if the driver uses it or not. And because there is no way we will be able to think about every possible use cases popping in the future, and it does not make backward compatibility to be a little more open here. > > > 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. > > Right. Since the coda driver does not support frame skipping yet, I'll > return -ENOTTY for decoder ENUM_FRAMEINTERVALS. > > regards > Philipp
On Fri, Apr 12, 2019 at 12:53 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > Le jeudi 11 avril 2019 à 14:00 +0200, Philipp Zabel a écrit : > > On Thu, 2019-04-11 at 12:18 +0200, Hans Verkuil wrote: > > > 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). > > > > Those are the limits imposed by the frame rate registers. > > > > > I would probably return 1 fps to 200 fps (200 Hz is the highest framerate > > > defined by CTA-861-G). > > > > Why? We can certainly encode more than 200 fps in real time if only the > > frame size is small enough, and for offline transcoding the nominal > > frame rate can be anything the coded format supports. > > Smaller than 1 fps might be useful for time lapse videos as well. > > > > > This can also eventually be a helper function in v4l2-mem2mem.c since I expect > > > this to be the same for all (?) encoders. > > > > For example h.264 has an optional 32-bit numerator and denominator > > (num_units_in_tick and time_scale in the Video Usability Information). > > I don't see any reason to limit this artificially in the driver. > > > > > > 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? > > > > That is a difficult question. Yes, depending on CODA hardware variant, > > codec, and actual bitstream (where this information is optional), the > > firmware may or may not report the frame rate. Maybe it is fundamentally > > not a good idea to try to control frame skipping via the nominal frame > > rate instead of directly. > > The Xilinx VCU decoder will allocate a different amount of cores per > session depending on the framerate we give it. This is how they manage > to handle more stream for lower rate/resolution etc. And it makes the > S_PARM call mandatory, since faking one will always lead to some > streams not to work properly, or less streams being handled in > parallel. > > I personally don't see why we should in anyway be restrictive to the > information we are allowed to pass to a driver. Specially that is does > not matter if the driver uses it or not. And because there is no way we > will be able to think about every possible use cases popping in the > future, and it does not make backward compatibility to be a little more > open here. I think it just boils down to defining the meaning properly, especially thinking about what ENUM_FRAME_INTERVALS would return. In particular, I don't think there is any sensible way to guarantee any performance aspects related to the frame rate conveyed via this channel, so even if the application sees 200 fps here, it should just mean that the bit-rate control algorithm can handle 200 fps streams, but it can actually encode 60 fps (and so not real-time), due to the hardware performance. Best regards, Tomasz
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 943f003c26c4..2966eb1c4d2d 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -1117,7 +1117,8 @@ static int coda_enum_frameintervals(struct file *file, void *fh, struct v4l2_frmivalenum *f) { struct coda_ctx *ctx = fh_to_ctx(fh); - int i; + struct coda_q_data *q_data; + const struct coda_codec *codec; if (f->index) return -EINVAL; @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh, if (!ctx->vdoa && f->pixel_format == V4L2_PIX_FMT_YUYV) return -EINVAL; - for (i = 0; i < CODA_MAX_FORMATS; i++) { - if (f->pixel_format == ctx->cvd->src_formats[i] || - f->pixel_format == ctx->cvd->dst_formats[i]) - break; + if (ctx->inst_type == CODA_INST_ENCODER) { + if (coda_format_normalize_yuv(f->pixel_format) == + V4L2_PIX_FMT_YUV420) { + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); + codec = coda_find_codec(ctx->dev, f->pixel_format, + q_data->fourcc); + } else { + codec = coda_find_codec(ctx->dev, V4L2_PIX_FMT_YUV420, + f->pixel_format); + } + } else { + if (coda_format_normalize_yuv(f->pixel_format) == + V4L2_PIX_FMT_YUV420) { + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); + codec = coda_find_codec(ctx->dev, q_data->fourcc, + f->pixel_format); + } else { + codec = coda_find_codec(ctx->dev, f->pixel_format, + V4L2_PIX_FMT_YUV420); + } } - if (i == CODA_MAX_FORMATS) + if (!codec) + return -EINVAL; + + if (f->width < MIN_W || f->width > codec->max_w || + f->height < MIN_H || f->height > codec->max_h) return -EINVAL; f->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
Let VIDIOC_ENUM_FRAMEINTERVALS return -EINVAL if userspace queries frame intervals for unsupported frame sizes. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/platform/coda/coda-common.c | 33 ++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-)