Re: [PATCH 1/2] v4l2-mem2mem: add job_write callback

[Tomasz Figa <tfiga@chromium.org>]

Hi Hans,

On Mon, Jan 7, 2019 at 11:43 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Tomasz, Ezequiel, Philipp,
>
> I'd really like to have a review of this patch. If you have some time to
> look at this, then that would be very nice.
>

Sorry for the long delay. I'm just back from the holidays. (The end
year holiday season in Japan is quite long. ;))

Please see my comments inline.

> On a related note: I am also thinking of adding a new callback to help
> decoders search for headers containing the resolution. This as per the
> stateful decoder spec where you start streaming on the output queue
> until the header information is found. Only then will userspace start
> the capture queue.
>
> Currently the search for this header is done in buf_queue (e.g. mediatek)
> but it would be much nicer if this is properly integrated into the mem2mem
> framework.

Hmm, the feature makes sense, but I wonder if this wouldn't make the
mem2mem framework too much of a codec framework? Perhaps we need the
latter instead, building on top of the mem2mem framework?

>
> Anyway, that's just a heads-up.
>
> Regards,
>
>         Hans
>
> On 12/14/2018 04:43 PM, hverkuil-cisco@xs4all.nl wrote:
> > From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >
> > The m2m framework works well for a stateful decoder: in job_ready()
> > you can process all output buffers until the whole compressed frame
> > is available for decoding, and then you return true to signal that
> > the decoder can start. The decoder decodes to a single capture buffer,
> > and the job is finished.
> >
> > For encoders, however, life is harder: currently the m2m framework
> > assumes that the result of the encoder fits in a single buffer. There
> > is no nice API to be able to store the compressed frames into multiple
> > capture buffers.
> >
> > This patch adds a new mode (TRANS_WRITING) where the result of the
> > device_run is written out buffer-by-buffer until all the data is
> > written. At that time v4l2_m2m_job_finish() is called and the next
> > job can start.
> >
> > This mode is triggered by calling v4l2_m2m_job_writing() if it is
> > clear in the process step that multiple buffers are required.
> >

I'm wondering how this plays with the Stateful Encoder Interface. We
defined it as below:

    "A stateful video encoder takes raw video frames in display order
     and encodes them into a bitstream. It generates complete chunks
     of the bitstream, including all metadata, headers, etc. The
     resulting bitstream does not require any further post-processing
     by the client."

Although not explicitly said (maybe it should be), my understanding
was that we want complete frames to be encoded into the buffers, with
the userspace being responsible for allocating buffers big enough (or
possibly adding bigger buffers at runtime using CREATE_BUFS, if
needed).

In Chromium, we currently expect the above to hold and this feature
wouldn't impact it, since we can just keep allocating buffers "big
enough". If we intend to optimize things, though, we would need to
know that the buffers refer to the same frame, so we can put it in the
same target buffer for the higher layers of processing.

I think this all needs to be defined in the Stateful Encoder Interface.

Otherwise, the patch is:

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz