On Fri, Oct 19, 2018 at 10:00 PM Ezequiel Garcia <ezequ...@collabora.com> wrote:
>
> On Fri, 2018-10-19 at 09:14 +0200, Hans Verkuil wrote:
> > On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> > > The decoder interface (not yet merged) specifies that
> > > width and height values set on the OUTPUT queue, must
> > > be propagated to the CAPTURE queue.
> > >
> > > This is not enough to comply with the specification,
> > > which would require to properly support stream resolution
> > > changes detection and notification.
> > >
> > > However, it's a relatively small change, which fixes behavior
> > > required by some applications such as gstreamer.
> > >
> > > With this change, it's possible to run a simple T(T⁻¹) pipeline:
> > >
> > > gst-launch-1.0 videotestsrc ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink
> > >
> > > Signed-off-by: Ezequiel Garcia <ezequ...@collabora.com>
> > > ---
> > > drivers/media/platform/vicodec/vicodec-core.c | 15 +++++++++++++++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/vicodec/vicodec-core.c
> > > b/drivers/media/platform/vicodec/vicodec-core.c
> > > index 1eb9132bfc85..a2c487b4b80d 100644
> > > --- a/drivers/media/platform/vicodec/vicodec-core.c
> > > +++ b/drivers/media/platform/vicodec/vicodec-core.c
> > > @@ -673,6 +673,13 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx,
> > > struct v4l2_format *f)
> > > q_data->width = pix->width;
> > > q_data->height = pix->height;
> > > q_data->sizeimage = pix->sizeimage;
> > > +
> > > + /* Propagate changes to CAPTURE queue */
> > > + if (!ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type)) {
> >
> > Do we need !ctx->is_enc? Isn't this the same for both decoder and encoder?
> >
>
> Well, I wasn't really sure about this. The decoder document clearly
> says that changes has to be propagated to the capture queue, but that
> statement
> is not in the encoder spec.
>
> Since gstreamer didn't needs this, I decided not to add it.
>
> Perhaps it's something to correct in the encoder spec?
Hmm, in the v2 of the documentation I sent recently, the CAPTURE queue
of an encoder doesn't have width and height specified. For formats
that have the resolution embedded in bitstream metadata, this isn't
anything that the userspace should be concerned with. I forgot about
the formats that don't have the resolution in the metadata, so we
might need to bring them back. Then the propagation would have to be
there indeed.
> > > + ctx->q_data[V4L2_M2M_DST].width = pix->width;
> > > + ctx->q_data[V4L2_M2M_DST].height = pix->height;
> > > + ctx->q_data[V4L2_M2M_DST].sizeimage = pix->sizeimage;
> >
> > This is wrong: you are copying the sizeimage for the compressed format as
> > the
> > sizeimage for the raw format, which is quite different.
> >
>
> Doh, you are right.
>
> > I think you need to make a little helper function that can update the
> > width/height
> > of a particular queue and that can calculate the sizeimage correctly.
> >
I wish we had generic helpers to manage all the formats in one place,
rather than duplicating the handling in each driver. I found many
cases of drivers not reporting bytesperline correctly or not handling
some formats (other than default and so often not tested) correctly.
If we could just have the driver call
v4l2_fill_pixfmt_mp_for_format(&pixfmt_mp, pixelformat, width, height,
...), a lot of boilerplate and potential source of errors could be
removed. (Bonus points for helpers that can convert pixfmt_mp for a
non-M format, e.g. NV12, into a pixfmt_mp for the corresponding M
format, e.g. NV12M, so that all the drivers that can support M formats
can also handle non-M formats automatically.)
One thing to note, though, is that there might be driver specific
alignment constraints in the play, so care must be taken.
Best regards,
Tomasz
