diff mbox series

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

Message ID 20200308002305.10822-1-andriy.gelman@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] avcodec/v4l2_m2m_enc: Adapt to the new internal encode API | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork fail Make failed

Commit Message

Andriy Gelman March 8, 2020, 12:23 a.m. UTC 
From: Andriy Gelman <andriy.gelman@gmail.com>

Should be squashed with:
http://ffmpeg.org/pipermail/ffmpeg-devel/2020-February/257735.html

Signed-off-by: Andriy Gelman <andriy.gelman@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(-)

Comments

James Almer March 8, 2020, 2:13 a.m. UTC | #1 
On 3/7/2020 9:23 PM, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@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@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.

>      if (ret < 0)
>          return ret;
>  
> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> index c9f1741bfd0..22b0cbd27ac 100644
> --- a/libavcodec/v4l2_m2m_enc.c
> +++ b/libavcodec/v4l2_m2m_enc.c
> @@ -24,6 +24,7 @@
>  #include <linux/videodev2.h>
>  #include <sys/ioctl.h>
>  #include <search.h>
> +#include "encode.h"
>  #include "libavcodec/avcodec.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/pixfmt.h"
> @@ -259,11 +260,24 @@ static int v4l2_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
>      V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
>      V4L2Context *const capture = &s->capture;
>      V4L2Context *const output = &s->output;
> +    AVFrame *frame = s->frame;
>      int ret;
>  
>      if (s->draining)
>          goto dequeue;
>  
> +    ret = ff_encode_get_frame(avctx, frame);
> +    if (ret < 0 && ret != AVERROR_EOF)
> +        return ret;
> +
> +    if (ret == AVERROR_EOF)
> +        frame = NULL;
> +
> +    ret = v4l2_send_frame(avctx, frame);
> +    av_frame_unref(frame);
> +    if (ret < 0)
> +        return ret;
> +
>      if (!output->streamon) {
>          ret = ff_v4l2_context_set_status(output, VIDIOC_STREAMON);
>          if (ret) {
> @@ -293,7 +307,7 @@ static av_cold int v4l2_encode_init(AVCodecContext *avctx)
>      uint32_t v4l2_fmt_output;
>      int ret;
>  
> -    ret = ff_v4l2_m2m_create_context(priv, &s);
> +    ret = ff_v4l2_m2m_create_context(priv, !av_codec_is_decoder(avctx->codec), &s);
>      if (ret < 0)
>          return ret;
>  
> @@ -367,7 +381,6 @@ static const AVOption options[] = {
>          .priv_data_size = sizeof(V4L2m2mPriv), \
>          .priv_class     = &v4l2_m2m_ ## NAME ##_enc_class, \
>          .init           = v4l2_encode_init, \
> -        .send_frame     = v4l2_send_frame, \
>          .receive_packet = v4l2_receive_packet, \
>          .close          = v4l2_encode_close, \
>          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
>
James Almer March 8, 2020, 2:23 a.m. UTC | #2 
On 3/7/2020 9:23 PM, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
> 
> Should be squashed with:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2020-February/257735.html
> 
> Signed-off-by: Andriy Gelman <andriy.gelman@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);
>      if (ret < 0)
>          return ret;
>  
> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> index c9f1741bfd0..22b0cbd27ac 100644
> --- a/libavcodec/v4l2_m2m_enc.c
> +++ b/libavcodec/v4l2_m2m_enc.c
> @@ -24,6 +24,7 @@
>  #include <linux/videodev2.h>
>  #include <sys/ioctl.h>
>  #include <search.h>
> +#include "encode.h"
>  #include "libavcodec/avcodec.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/pixfmt.h"
> @@ -259,11 +260,24 @@ static int v4l2_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
>      V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
>      V4L2Context *const capture = &s->capture;
>      V4L2Context *const output = &s->output;
> +    AVFrame *frame = s->frame;
>      int ret;
>  
>      if (s->draining)
>          goto dequeue;
>  
> +    ret = ff_encode_get_frame(avctx, frame);
> +    if (ret < 0 && ret != AVERROR_EOF)
> +        return ret;
> +
> +    if (ret == AVERROR_EOF)
> +        frame = NULL;
> +
> +    ret = v4l2_send_frame(avctx, frame);

What happens if ff_encode_get_frame() returns AVERROR(EAGAIN)? I don't
think ff_v4l2_context_enqueue_frame() is meant to receive empty,
non-NULL frames.

> +    av_frame_unref(frame);
> +    if (ret < 0)
> +        return ret;
> +
>      if (!output->streamon) {
>          ret = ff_v4l2_context_set_status(output, VIDIOC_STREAMON);
>          if (ret) {
> @@ -293,7 +307,7 @@ static av_cold int v4l2_encode_init(AVCodecContext *avctx)
>      uint32_t v4l2_fmt_output;
>      int ret;
>  
> -    ret = ff_v4l2_m2m_create_context(priv, &s);
> +    ret = ff_v4l2_m2m_create_context(priv, !av_codec_is_decoder(avctx->codec), &s);
>      if (ret < 0)
>          return ret;
>  
> @@ -367,7 +381,6 @@ static const AVOption options[] = {
>          .priv_data_size = sizeof(V4L2m2mPriv), \
>          .priv_class     = &v4l2_m2m_ ## NAME ##_enc_class, \
>          .init           = v4l2_encode_init, \
> -        .send_frame     = v4l2_send_frame, \
>          .receive_packet = v4l2_receive_packet, \
>          .close          = v4l2_encode_close, \
>          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
>
Andriy Gelman March 8, 2020, 4:33 a.m. UTC | #3 
On Sat, 07. Mar 23:23, James Almer wrote:
> On 3/7/2020 9:23 PM, Andriy Gelman wrote:
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> > 
> > Should be squashed with:
> > http://ffmpeg.org/pipermail/ffmpeg-devel/2020-February/257735.html
> > 
> > Signed-off-by: Andriy Gelman <andriy.gelman@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);
> >      if (ret < 0)
> >          return ret;
> >  
> > diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> > index c9f1741bfd0..22b0cbd27ac 100644
> > --- a/libavcodec/v4l2_m2m_enc.c
> > +++ b/libavcodec/v4l2_m2m_enc.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/videodev2.h>
> >  #include <sys/ioctl.h>
> >  #include <search.h>
> > +#include "encode.h"
> >  #include "libavcodec/avcodec.h"
> >  #include "libavutil/pixdesc.h"
> >  #include "libavutil/pixfmt.h"
> > @@ -259,11 +260,24 @@ static int v4l2_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
> >      V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
> >      V4L2Context *const capture = &s->capture;
> >      V4L2Context *const output = &s->output;
> > +    AVFrame *frame = s->frame;
> >      int ret;
> >  
> >      if (s->draining)
> >          goto dequeue;
> >  
> > +    ret = ff_encode_get_frame(avctx, frame);
> > +    if (ret < 0 && ret != AVERROR_EOF)
> > +        return ret;
> > +
> > +    if (ret == AVERROR_EOF)
> > +        frame = NULL;
> > +
> > +    ret = v4l2_send_frame(avctx, frame);
> 
> What happens if ff_encode_get_frame() returns AVERROR(EAGAIN)? I don't
> think ff_v4l2_context_enqueue_frame() is meant to receive empty,
> non-NULL frames.
> 

I don't think that's possible because the if condition catches 
AVERROR(EAGAIN) and returns it.

> > +    av_frame_unref(frame);
> > +    if (ret < 0)
> > +        return ret;
> > +
> >      if (!output->streamon) {
> >          ret = ff_v4l2_context_set_status(output, VIDIOC_STREAMON);
> >          if (ret) {
> > @@ -293,7 +307,7 @@ static av_cold int v4l2_encode_init(AVCodecContext *avctx)
> >      uint32_t v4l2_fmt_output;
> >      int ret;
> >  
> > -    ret = ff_v4l2_m2m_create_context(priv, &s);
> > +    ret = ff_v4l2_m2m_create_context(priv, !av_codec_is_decoder(avctx->codec), &s);
> >      if (ret < 0)
> >          return ret;
> >  
> > @@ -367,7 +381,6 @@ static const AVOption options[] = {
> >          .priv_data_size = sizeof(V4L2m2mPriv), \
> >          .priv_class     = &v4l2_m2m_ ## NAME ##_enc_class, \
> >          .init           = v4l2_encode_init, \
> > -        .send_frame     = v4l2_send_frame, \
> >          .receive_packet = v4l2_receive_packet, \
> >          .close          = v4l2_encode_close, \
> >          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
> >
Andriy Gelman March 8, 2020, 5:40 a.m. UTC | #4 
On Sat, 07. Mar 23:13, James Almer wrote:
> On 3/7/2020 9:23 PM, Andriy Gelman wrote:
> > From: Andriy Gelman <andriy.gelman@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@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.
Lynne March 8, 2020, 12:29 p.m. UTC | #5 
Mar 8, 2020, 05:40 by andriy.gelman@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@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@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.
James Almer March 8, 2020, 1:43 p.m. UTC | #6 
On 3/8/2020 9:29 AM, Lynne wrote:
> Mar 8, 2020, 05:40 by andriy.gelman@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@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@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

Removing the dependency of the new public encode API
(avcodec_send_frame/avcodec_receive_packet) on the old public encode API
(avcodec_encode_audio2/avcodec_encode_video2) so the latter may finally
be removed is one of the core objectives of this patchset.
You mentioned you wanted that to happen in the upcoming major bump.
While it's not going to be the case, it wouldn't even be possible
without this change.

> 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.

Having a single internal callback function that fetches frames as
needed, requests the user when there are none, and outputs packets as
they are available is easier to implement than two callbacks where
you're the one that needs to take extra precautions about when to return
EAGAIN from one or the other to prevent the API violation scenario of
both returning that error code.

> Sure, it'll take time to port over all the old codecs to the new API (so the wrapping code can be removed)

No one is going to port dozens of encoders to the new internal API when
they are all 1:1. It's wasted work. The old internal callback is not
going away.

, 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.

How is it discouraged? librav1e is a new encoder and it uses the new API
because it's ideal for it, as does almost every hardware and external
library encoder.
Hell, the libaom/libvpx wrappers should be ported to it in order to
remove the awful buffering of returned packets they feature. But the
mpeg2 encoder? That one is ok using AVCodec.encode2()

> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

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);
     if (ret < 0)
         return ret;
 
diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
index c9f1741bfd0..22b0cbd27ac 100644
--- a/libavcodec/v4l2_m2m_enc.c
+++ b/libavcodec/v4l2_m2m_enc.c
@@ -24,6 +24,7 @@ 
 #include <linux/videodev2.h>
 #include <sys/ioctl.h>
 #include <search.h>
+#include "encode.h"
 #include "libavcodec/avcodec.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/pixfmt.h"
@@ -259,11 +260,24 @@  static int v4l2_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
     V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
     V4L2Context *const capture = &s->capture;
     V4L2Context *const output = &s->output;
+    AVFrame *frame = s->frame;
     int ret;
 
     if (s->draining)
         goto dequeue;
 
+    ret = ff_encode_get_frame(avctx, frame);
+    if (ret < 0 && ret != AVERROR_EOF)
+        return ret;
+
+    if (ret == AVERROR_EOF)
+        frame = NULL;
+
+    ret = v4l2_send_frame(avctx, frame);
+    av_frame_unref(frame);
+    if (ret < 0)
+        return ret;
+
     if (!output->streamon) {
         ret = ff_v4l2_context_set_status(output, VIDIOC_STREAMON);
         if (ret) {
@@ -293,7 +307,7 @@  static av_cold int v4l2_encode_init(AVCodecContext *avctx)
     uint32_t v4l2_fmt_output;
     int ret;
 
-    ret = ff_v4l2_m2m_create_context(priv, &s);
+    ret = ff_v4l2_m2m_create_context(priv, !av_codec_is_decoder(avctx->codec), &s);
     if (ret < 0)
         return ret;
 
@@ -367,7 +381,6 @@  static const AVOption options[] = {
         .priv_data_size = sizeof(V4L2m2mPriv), \
         .priv_class     = &v4l2_m2m_ ## NAME ##_enc_class, \
         .init           = v4l2_encode_init, \
-        .send_frame     = v4l2_send_frame, \
         .receive_packet = v4l2_receive_packet, \
         .close          = v4l2_encode_close, \
         .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \