[FFmpeg-devel] [PATCH] avcodec/v4l2_m2m_enc: Adapt to the new internal encode API
James Almer
jamrial at gmail.com
Sun Mar 8 15:43:34 EET 2020
On 3/8/2020 9:29 AM, Lynne wrote:
> 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
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list