[FFmpeg-devel] [PATCH] avcodec/v4l2_m2m_enc: Adapt to the new internal encode API

[Lynne]

Mar 8, 2020, 05:40 by andriy.gelman at gmail.com:

> On Sat, 07. Mar 23:13, James Almer wrote:
>
>> On 3/7/2020 9:23 PM, Andriy Gelman wrote:
>> > From: Andriy Gelman <andriy.gelman at gmail.com>
>> > 
>> > Should be squashed with:
>> > http://ffmpeg.org/pipermail/ffmpeg-devel/2020-February/257735.html
>>
>> Thank you! Only three remain now :)
>>
>> > 
>> > Signed-off-by: Andriy Gelman <andriy.gelman at gmail.com>
>> > ---
>> >  libavcodec/v4l2_m2m.c     |  9 ++++++++-
>> >  libavcodec/v4l2_m2m.h     |  6 +++++-
>> >  libavcodec/v4l2_m2m_dec.c |  2 +-
>> >  libavcodec/v4l2_m2m_enc.c | 17 +++++++++++++++--
>> >  4 files changed, 29 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
>> > index 2d21f910bcc..1799837fb9f 100644
>> > --- a/libavcodec/v4l2_m2m.c
>> > +++ b/libavcodec/v4l2_m2m.c
>> > @@ -329,6 +329,7 @@ static void v4l2_m2m_destroy_context(void *opaque, uint8_t *context)
>> >      sem_destroy(&s->refsync);
>> > 
>> >      close(s->fd);
>> > +    av_frame_free(&s->frame);
>> > 
>> >      av_free(s);
>> >  }
>> > @@ -394,7 +395,7 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
>> >      return v4l2_configure_contexts(s);
>> >  }
>> > 
>> > -int ff_v4l2_m2m_create_context(V4L2m2mPriv *priv, V4L2m2mContext **s)
>> > +int ff_v4l2_m2m_create_context(V4L2m2mPriv *priv, int is_encoder, V4L2m2mContext **s)
>> >  {
>> >      *s = av_mallocz(sizeof(V4L2m2mContext));
>> >      if (!*s)
>> > @@ -417,5 +418,11 @@ int ff_v4l2_m2m_create_context(V4L2m2mPriv *priv, V4L2m2mContext **s)
>> >      priv->context->self_ref = priv->context_ref;
>> >      priv->context->fd = -1;
>> > 
>> > +    if (is_encoder) {
>> > +        priv->context->frame = av_frame_alloc();
>> > +        if (!priv->context->frame)
>> > +            return AVERROR(ENOMEM);
>> > +    }
>> > +
>> >      return 0;
>> >  }
>> > diff --git a/libavcodec/v4l2_m2m.h b/libavcodec/v4l2_m2m.h
>> > index 456281f48c5..5d6106224dd 100644
>> > --- a/libavcodec/v4l2_m2m.h
>> > +++ b/libavcodec/v4l2_m2m.h
>> > @@ -58,6 +58,9 @@ typedef struct V4L2m2mContext {
>> >      int draining;
>> >      AVPacket buf_pkt;
>> > 
>> > +    /* Reference to a frame. Only used during encoding */
>> > +    AVFrame *frame;
>> > +
>> >      /* Reference to self; only valid while codec is active. */
>> >      AVBufferRef *self_ref;
>> > 
>> > @@ -79,11 +82,12 @@ typedef struct V4L2m2mPriv {
>> >   * Allocate a new context and references for a V4L2 M2M instance.
>> >   *
>> >   * @param[in] ctx The V4L2m2mPriv instantiated by the encoder/decoder.
>> > + * @param[in] is_encoder Whether the context is for encoder/decoder
>> >   * @param[out] ctx The V4L2m2mContext.
>> >   *
>> >   * @returns 0 in success, a negative error code otherwise.
>> >   */
>> > -int ff_v4l2_m2m_create_context(V4L2m2mPriv *priv, V4L2m2mContext **s);
>> > +int ff_v4l2_m2m_create_context(V4L2m2mPriv *priv, int is_encoder, V4L2m2mContext **s);
>> > 
>> > 
>> >  /**
>> > diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
>> > index d666edffe46..7c7ac2de8c5 100644
>> > --- a/libavcodec/v4l2_m2m_dec.c
>> > +++ b/libavcodec/v4l2_m2m_dec.c
>> > @@ -181,7 +181,7 @@ static av_cold int v4l2_decode_init(AVCodecContext *avctx)
>> >      V4L2m2mPriv *priv = avctx->priv_data;
>> >      int ret;
>> > 
>> > -    ret = ff_v4l2_m2m_create_context(priv, &s);
>> > +    ret = ff_v4l2_m2m_create_context(priv, !av_codec_is_decoder(avctx->codec), &s);
>>
>>
>> There's av_codec_is_encoder(). Also, You could pass avctx to
>> ff_v4l2_m2m_create_context() instead of priv, and call
>> av_codec_is_encoder() there, saving yourself the extra parameter.
>>
>> Alternatively, just allocate the frame unconditionally and avoid all
>> this extra code. ff_v4l2_m2m_create_context() is called once during
>> init, so it's not an issue if the decoder allocates it as well.
>>
>
> That's true, thanks.
> I prefer your second option a bit more because it keeps same style in
> v4l2_m2m.h.
>
> I'll resend tomorrow in case there are more comments.
>

To be honest, I'm against the new internal API.
I'd rather keep the new API as a wrapper for the old API unless the codec uses the new API if it means we can have asynchronous encoders and decoders. The new API eliminates that since codec threads would have to resort to polling for new packets/frames. With the old send/receive API those functions would simply be queuing and fetching from FIFO buffers the codec main thread retrieves/populates.
Sure, it'll take time to port over all the old codecs to the new API (so the wrapping code can be removed), but the only reason no new software encoders have used the new API was simply because it was discouraged. Otherwise I'd have used the new API in any new software codecs I've written and I'd have started porting codecs to the new API.
Not to mention the porting can be hugely simplified by a few dozen lines of helpers to automatically make FIFOs and invoke the old single function.