[FFmpeg-devel] [PATCH 2/2] libavcodec/v4l2: use libdrm

Zhao Zhili quinkblack at foxmail.com
Wed Dec 18 09:16:26 EET 2024



> On Dec 18, 2024, at 12:23, Scott Theisen <scott.the.elm at gmail.com> wrote:
> 
> On 12/17/24 11:10, Zhao Zhili wrote:
>>> On Dec 17, 2024, at 06:48, Scott Theisen <scott.the.elm at gmail.com> wrote:
>>> 
>>> On 12/15/24 22:15, Zhao Zhili wrote:
>>>>> On Dec 15, 2024, at 12:14, Scott Theisen<scott.the.elm at gmail.com> wrote:
>>>>> 
>>>>> Based on patches by Lukas Rusak from
>>>>> https://github.com/lrusak/FFmpeg/commits/v4l2-drmprime-v4
>>>>> 
>>>>> libavcodec: v4l2m2m: output AVDRMFrameDescriptor
>>>>> https://github.com/lrusak/FFmpeg/commit/2cb8052ac65a56d8a3f347a1e6f12d4449a5a614
>>>>> 
>>>>> libavcodec: v4l2m2m: depends on libdrm
>>>>> https://github.com/lrusak/FFmpeg/commit/ab4cf3e6fb37cffdebccca52e36a7b2deb7e729f
>>>>> 
>>>>> libavcodec: v4l2m2m: set format_modifier to DRM_FORMAT_MOD_LINEAR
>>>>> https://github.com/lrusak/FFmpeg/commit/9438a6efa29c7c7ec80c39c9b013b9a12d7b5f33
>>>>> 
>>>>> libavcodec: v4l2m2m: only mmap the buffer when it is output type and drm prime is used
>>>>> https://github.com/lrusak/FFmpeg/commit/093656607863e47630de2d1cfcf0ac8e4b93a69e
>>>>> 
>>>>> libavcodec: v4l2m2m: allow using software pixel formats
>>>>> https://github.com/lrusak/FFmpeg/commit/8405b573e83838e6b2fea99825fbef32ee9f7767
>>>>> 
>>>>> libavcodec: v4l2m2m: implement hwcontext
>>>>> https://github.com/lrusak/FFmpeg/commit/b2c1f1eb39b54bf034497a7f2a7f23855d0a7cde
>>>>> 
>>>>> libavcodec: v4l2m2m: implement flush
>>>>> https://github.com/lrusak/FFmpeg/commit/e793ef82727d6d6f55c40844463d476e7e84efad
>>>>> 
>>>>> Originally added to MythTV in:
>>>>> FFmpeg: Patch FFmpeg for V4L2 codecs DRM PRIME support
>>>>> https://github.com/MythTV/mythtv/commit/cc7572f9b26189ad5d5d504c05f08e53e4e61b54
>>>>> 
>>>>> FFmpeg: Re-apply v4l2 memory to memory DRM_PRIME support
>>>>> https://github.com/MythTV/mythtv/commit/1c942720591b5b7820abe9ed0d805afabbdffe3c
>>>>> 
>>>>> modified in:
>>>>> V4L2 Codecs: Fix lockup when seeking
>>>>> https://github.com/MythTV/mythtv/commit/fdc0645aba9a9ad373888bd62ebcbc83a3feb7e5
>>>>> 
>>>>> v4l2_buffers: Add some libdrm ifdef's
>>>>> https://github.com/MythTV/mythtv/commit/336df1067abfa4fe7cf611541e5b6f3561fc81a2
>>>>> 
>>>>> !!!!
>>>>> NB: libavcodec/v4l2_m2m_dec.c: v4l2_decode_init(): I'm returning -1
>>>>> since I don't know what error code I should use.
>>>>> 
>>>>> Note also Lucas Rusak's v5 series:
>>>>> closer diff to current state, otherwise unchanged
>>>>> libavcodec: v4l2m2m: output AVDRMFrameDescriptor
>>>>> https://github.com/lrusak/FFmpeg/commit/c6b85ed30f06ea99513b13cc768a922ebe4d68c2
>>>>> 
>>>>> new option:
>>>>> libavcodec: v4l2m2m: add option to specify pixel format used by the decoder
>>>>> https://github.com/lrusak/FFmpeg/commit/ffc4419f456c00ab71cf93f792b0473c6de14e64
>>>>> 
>>>>> additional code vs v4
>>>>> libavcodec: v4l2m2m: implement flush
>>>>> https://github.com/lrusak/FFmpeg/commit/8595d06d4909bbec0aa14625fcfc869c6bcef696
>>>>> ---
>>>>> configure                 |   1 +
>>>>> libavcodec/v4l2_buffers.c | 206 +++++++++++++++++++++++++++++++++++---
>>>>> libavcodec/v4l2_buffers.h |   4 +
>>>>> libavcodec/v4l2_context.c |  43 +++++++-
>>>>> libavcodec/v4l2_context.h |   2 +
>>>>> libavcodec/v4l2_m2m.h     |   5 +
>>>>> libavcodec/v4l2_m2m_dec.c |  62 ++++++++++++
>>>>> 7 files changed, 305 insertions(+), 18 deletions(-)
>>>>> 
>>>>> diff --git a/configure b/configure
>>>>> index bf55ba67fa..5f02cf3b51 100755
>>>>> --- a/configure
>>>>> +++ b/configure
>>>>> @@ -3770,6 +3770,7 @@ sndio_indev_deps="sndio"
>>>>> sndio_outdev_deps="sndio"
>>>>> v4l2_indev_deps_any="linux_videodev2_h sys_videoio_h"
>>>>> v4l2_indev_suggest="libv4l2"
>>>>> +v4l2_outdev_deps="libdrm"
>>>> Why v4l2_outdev when the patch is for libavcodec?
>>> Lukas Rusak appears to have only submitted up to v3 of his patches in 2018.
>>> 
>>> This appears to be from a comment in https://patchwork.ffmpeg.org/project/ffmpeg/patch/4bbeff66-6123-9296-5775-96d75809eee0@jkqxz.net/
>>> 
>>> However, with the `#if CONFIG_LIBDRM`s added by Mark Kendall for MythTV, I don't think it is a hard dependency.
>>> 
>>> ...
>>>>> diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
>>>>> index aa2d759e1e..cfeee7f2fb 100644
>>>>> --- a/libavcodec/v4l2_m2m_dec.c
>>>>> +++ b/libavcodec/v4l2_m2m_dec.c
>>>>> @@ -23,12 +23,17 @@
>>>>> 
>>>>> #include <linux/videodev2.h>
>>>>> #include <sys/ioctl.h>
>>>>> +
>>>>> +#include "libavutil/hwcontext.h"
>>>>> +#include "libavutil/hwcontext_drm.h"
>>>>> #include "libavutil/pixfmt.h"
>>>>> #include "libavutil/pixdesc.h"
>>>>> #include "libavutil/opt.h"
>>>>> #include "libavcodec/avcodec.h"
>>>>> #include "codec_internal.h"
>>>>> #include "libavcodec/decode.h"
>>>>> +#include "libavcodec/internal.h"
>>>>> +#include "libavcodec/hwconfig.h"
>>>>> 
>>>>> #include "v4l2_context.h"
>>>>> #include "v4l2_m2m.h"
>>>>> @@ -205,7 +210,44 @@ static av_cold int v4l2_decode_init(AVCodecContext *avctx)
>>>>>     capture->av_codec_id = AV_CODEC_ID_RAWVIDEO;
>>>>>     capture->av_pix_fmt = avctx->pix_fmt;
>>>>> 
>>>>> +    /* the client requests the codec to generate DRM frames:
>>>>> +     *   - data[0] will therefore point to the returned AVDRMFrameDescriptor
>>>>> +     *       check the ff_v4l2_buffer_to_avframe conversion function.
>>>>> +     *   - the DRM frame format is passed in the DRM frame descriptor layer.
>>>>> +     *       check the v4l2_get_drm_frame function.
>>>>> +     */
>>>>> +    {
>>>>> +    const enum AVPixelFormat *pix_fmts = NULL;
>>>>> +    int num_pix_fmts = 0;
>>>>> +    ret = avcodec_get_supported_config(avctx, NULL, AV_CODEC_CONFIG_PIX_FORMAT,
>>>>> +                                       0, (const void **) &pix_fmts, &num_pix_fmts);
>>>> I don’t think here is the meant use case for  avcodec_get_supported_config.
>>>> Specify available pixel format directly in pix_fmts.
>>>> 
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +    if (pix_fmts == NULL || num_pix_fmts < 1)
>>>>> +        return -1;
>>>> There is no which error code to use issue after remove avcodec_get_supported_config.
>>>> 
>>> AVCodec::pix_fmts was deprecated, so I replaced it with avcodec_get_supported_config() in
>>> https://github.com/ulmus-scott/FFmpeg/commit/7ac4b4fc2282b4fdda78ffb1a446ceb85b681661
>>> 
>>> Was that incorrect?
>> Firstly, AVCodec::pix_fmts was deprecated, which should be replaced by FFCodec::get_supported_config
>> callback for **internal implementation**. avcodec_get_supported_config is towards for libavcodec API users.
> 
> I don't see any problem with calling the public API internally.
> 
>> Secondly, AVCodec::pix_fmts and FFCodec::get_supported_config is for **encoder**. For decoder, it’s
>> AVCodecContext::get_format callback to do the negotiate, for example, h264_slice.c get_pixel_format().
> 
> OK, the list would only conditionally contain AV_PIX_FMT_DRM_PRIME then?

The list should contain AV_PIX_FMT_DRM_PRIME and the soft pixel format.

> 
>> 
>>>>> +    switch (ff_get_format(avctx, pix_fmts)) {
>>>>> +    case AV_PIX_FMT_DRM_PRIME:
>>>>> +        s->output_drm = 1;
>>>>> +        break;
>>>>> +    case AV_PIX_FMT_NONE:
>>>>> +        return 0;
>>>>> +        break;
>>>> Why return 0 for AV_PIX_FMT_NONE? It’s suspicious.
>>>> ‘break’ can be removed.
>>> That is from https://github.com/lrusak/FFmpeg/commit/8405b573e83838e6b2fea99825fbef32ee9f7767
>>> 
>>> I agree return 0 is strange.  All I can say is that is what the code used by MythTV is and has been for years.  (Maybe MythTV always requests AV_PIX_FMT_DRM_PRIME?)
>>> 
>>> This change is not used by MythTV but maybe https://github.com/lrusak/FFmpeg/commit/ffc4419f456c00ab71cf93f792b0473c6de14e64 is related or an alternate way of requesting the pixel format?
>>> 
>>>>> +    default:
>>>>> +        break;
>>>>> +    }
>>>>> +    }
> 
> As the change currently stands, I think this is equivalent to `s->output_drm = 1;`.
> 
>>>>> +
>>>>> +    s->device_ref = av_hwdevice_ctx_alloc(AV_HWDEVICE_TYPE_DRM);
>>>>> +    if (!s->device_ref) {
>>>>> +        ret = AVERROR(ENOMEM);
>>>>> +        return ret;
>>>>> +    }
>>>> I’m not sure whether there is use case to let user specify device?
>>> This is from https://github.com/lrusak/FFmpeg/commit/b2c1f1eb39b54bf034497a7f2a7f23855d0a7cde
>>> 
>>> I don't know what you mean by "let user specify device".
>> User can create a DRM hw device directly via hwcontext API, and provided to decoder via
>> AVCodecContext::hw_device_ctx. It don’t looks right to always create hw device directly inside
>> the decoder implementation.
> 
> rkmppdec.c always creates its own hardware device context.

It’s not a good reference.

> 
>> 
>>>>> +
>>>>>     s->avctx = avctx;
>>>>> +    ret = av_hwdevice_ctx_init(s->device_ref);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>>     ret = ff_v4l2_m2m_codec_init(priv);
>>>>>     if (ret) {
>>>>>         av_log(avctx, AV_LOG_ERROR, "can't configure decoder\n");
> 
> So are you saying this should require the user to create the hardware context before calling avcodec_open2()?  And the change would then be something like this:

It’s “optional” for user to create the hardware context.

> ```
> s->output_drm = 0;
> #if CONFIG_LIBDRM
> if (avctx->hw_device_ctx != NULL && ((AVHWDeviceContext*)avctx->hw_device_ctx->data)->type == AV_HWDEVICE_TYPE_DRM)
>     s->output_drm = 1;
> #endif
> ```
> 
> What about AVCodecContext::hw_frames_ctx?

See the comments on AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX, AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX,
and so on.

You can take coviddec.c as reference. I just found it missing the AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX flag, but the
implementation is fine.

https://ffmpeg.org/pipermail/ffmpeg-devel/2024-December/337624.html

> 
>>>>> @@ -220,6 +262,16 @@ static av_cold int v4l2_decode_close(AVCodecContext *avctx)
>>>>>     return ff_v4l2_m2m_codec_end(avctx->priv_data);
>>>>> }
>>>>> 
>>>>> +static void v4l2_flush(AVCodecContext *avctx)
>>>>> +{
>>>>> +    V4L2m2mPriv *priv = avctx->priv_data;
>>>>> +    V4L2m2mContext* s = priv->context;
>>>>> +
>>>>> +    /* wait for pending buffer references */
>>>>> +    if (atomic_load(&s->refcount))
>>>>> +        while(sem_wait(&s->refsync) == -1 && errno == EINTR);
>>>>> +}
>>>> Does refcount depends on users behavior like holding a frame? Is it possible to run into deadlock here, e.g.,
>>>> user request flush while holding some frames?
>>> I don't know, but Lukas Rusak removed that in his v5 patch (not used by MythTV) https://github.com/lrusak/FFmpeg/commit/8595d06d4909bbec0aa14625fcfc869c6bcef696
>>> 
>>> Original v4 patch for reference: https://github.com/lrusak/FFmpeg/commit/e793ef82727d6d6f55c40844463d476e7e84efad
>>> 
>>>>> +
>>>>> #define OFFSET(x) offsetof(V4L2m2mPriv, x)
>>>>> #define FLAGS AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM
>>>>> 
>>>>> @@ -230,6 +282,11 @@ static const AVOption options[] = {
>>>>>     { NULL},
>>>>> };
>>>>> 
>>>>> +static const AVCodecHWConfigInternal *v4l2_m2m_hw_configs[] = {
>>>>> +    HW_CONFIG_INTERNAL(DRM_PRIME),
>>>> Depends on whether allow user to specify drm device.
>>> This is from the main patch https://github.com/lrusak/FFmpeg/commit/2cb8052ac65a56d8a3f347a1e6f12d4449a5a614
>>> 
>>> Unless this is used internally by FFmpeg, MythTV does not appear to use it.
>>> 
> 
> Regards,
> 
> Scott Theisen
> _______________________________________________
> 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