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 |
Context | Check | Description |
---|---|---|
andriy/ffmpeg-patchwork | fail | Make failed |
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, \ >
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, \ >
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, \ > >
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.
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.
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 --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, \