Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
On 17/08/18 04:36, Lukas Rusak wrote:
> On Sat, 2018-08-04 at 22:43 +0100, Mark Thompson wrote:
>> On 04/08/18 01:40, Lukas Rusak wrote:
>>> This allows for a zero-copy output by exporting the v4l2 buffer
>>> then wrapping that buffer
>>> in the AVDRMFrameDescriptor like it is done in rkmpp.
>>>
>>> This has been in use for quite some time with great success on many
>>> platforms including:
>>> - Amlogic S905
>>> - Raspberry Pi
>>> - i.MX6
>>> - Dragonboard 410c
>>>
>>> This was developed in conjunction with Kodi to allow handling the
>>> zero-copy buffer rendering.
>>> A simply utility for testing is also available here:
>>> https://github.com/BayLibre/ffmpeg-drm
>>>
>>> todo:
>>> - allow selecting pixel format output from decoder
>>> - allow configuring amount of output and capture buffers
>>>
>>> V2:
>>> - allow selecting AV_PIX_FMT_DRM_PRIME
>>>
>>> V3:
>>> - use get_format to select AV_PIX_FMT_DRM_PRIME
>>> - use hw_configs
>>> - add handling of AV_PIX_FMT_YUV420P format (for raspberry pi)
>>> - add handling of AV_PIX_FMT_YUYV422 format (for i.MX6 coda
>>> decoder)
>>> ---
>>> libavcodec/v4l2_buffers.c | 216
>>> --
>>> libavcodec/v4l2_buffers.h | 4 +
>>> libavcodec/v4l2_context.c | 40 ++-
>>> libavcodec/v4l2_m2m.c | 4 +-
>>> libavcodec/v4l2_m2m.h | 3 +
>>> libavcodec/v4l2_m2m_dec.c | 23
>>> 6 files changed, 253 insertions(+), 37 deletions(-)
>>> ...
>>> +
>>> static int v4l2_bufref_to_buf(V4L2Buffer *out, int plane, const
>>> uint8_t* data, int size, AVBufferRef* bref)
>>> {
>>> unsigned int bytesused, length;
>>> @@ -308,31 +442,43 @@ int ff_v4l2_buffer_buf_to_avframe(AVFrame
>>> *frame, V4L2Buffer *avbuf)
>>>
>>> av_frame_unref(frame);
>>>
>>> -/* 1. get references to the actual data */
>>> -for (i = 0; i < avbuf->num_planes; i++) {
>>> -ret = v4l2_buf_to_bufref(avbuf, i, >buf[i]);
>>> +if (buf_to_m2mctx(avbuf)->output_drm) {
>>> +/* 1. get references to the actual data */
>>> +ret = v4l2_buf_to_bufref_drm(avbuf, >buf[0]);
>>> if (ret)
>>> return ret;
>>>
>>> -frame->linesize[i] = avbuf->plane_info[i].bytesperline;
>>> -frame->data[i] = frame->buf[i]->data;
>>> -}
>>> +frame->data[0] = (uint8_t *) v4l2_get_drm_frame(avbuf);
>>> +frame->format = AV_PIX_FMT_DRM_PRIME;
>>
>> frame->hw_frames_ctx needs to be set here as well. (I think you can
>> use the same trivial device/frames context setup as in rkmppdec.c.)
>>
>
> Can you help me with this? This is the part I'm confused about. the
> v4l2 code here seems to use it's own reference counting so are we
> wanting to convert that to use the hw ctx instead or what is actually
> happening?
Working test patch below without changing anything else. Given that the V4L2
code already has its own custom reference counting I don't think there is any
point in changing that part. (Todo: put the initialisation in a more sensible
place, actually do cleanup, make the failure modes something other than
abort().)
With that change (and the other hacky patch), hwdownload does work on the
Odroid XU4 as, e.g.:
$ ./ffmpeg_g -y -c:v h264_v4l2m2m -hwaccel drm -hwaccel_device
/dev/dri/renderD128 -hwaccel_output_format drm_prime -i in.mp4 -an -vf
'hwdownload,format=nv21' -c:v libx264 out.mp4
(The output DRM objects are apparently both linear and CPU-mappable.)
Then trying to map to OpenCL to do something with it like:
$ ./ffmpeg_g -y -c:v h264_v4l2m2m -hwaccel drm -hwaccel_device
/dev/dri/renderD128 -hwaccel_output_format drm_prime -init_hw_device
opencl=cl:0.0 -filter_hw_device cl -i in.mp4 -an -vf hwmap,unsharp_opencl -f
null -
almost works, but not quite because the Mali T628 driver lacks the standard
cl_khr_image2d_from_buffer extension. Removing the extension check does get it
right up to a failure on creating the image from the imported buffer, though,
so the import is indeed working. (The same case is working with the T760 in
the RK3288, clearly I need a slightly newer test setup to get this working with
V4L2.)
>>> ...
>>> @@ -186,6 +192,15 @@ 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.
>>> + */
>>> +if (ff_get_format(avctx, avctx->codec->pix_fmts) ==
>>> AV_PIX_FMT_DRM_PRIME)
>>> +s->output_drm = 1;
>>
>> This list needs to contain the software pixfmts as well, so that the
>> user can pick from the correct list.
>>
>
> Is there a simple way to do
Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
Just an FYI i will be developing here if anyone wants to comment and/or PR other changes for V4. https://github.com/lrusak/FFmpeg/commits/v4l2-drmprime-v4 > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
On Sat, 2018-08-04 at 22:43 +0100, Mark Thompson wrote:
> On 04/08/18 01:40, Lukas Rusak wrote:
> > This allows for a zero-copy output by exporting the v4l2 buffer
> > then wrapping that buffer
> > in the AVDRMFrameDescriptor like it is done in rkmpp.
> >
> > This has been in use for quite some time with great success on many
> > platforms including:
> > - Amlogic S905
> > - Raspberry Pi
> > - i.MX6
> > - Dragonboard 410c
> >
> > This was developed in conjunction with Kodi to allow handling the
> > zero-copy buffer rendering.
> > A simply utility for testing is also available here:
> > https://github.com/BayLibre/ffmpeg-drm
> >
> > todo:
> > - allow selecting pixel format output from decoder
> > - allow configuring amount of output and capture buffers
> >
> > V2:
> > - allow selecting AV_PIX_FMT_DRM_PRIME
> >
> > V3:
> > - use get_format to select AV_PIX_FMT_DRM_PRIME
> > - use hw_configs
> > - add handling of AV_PIX_FMT_YUV420P format (for raspberry pi)
> > - add handling of AV_PIX_FMT_YUYV422 format (for i.MX6 coda
> > decoder)
> > ---
> > libavcodec/v4l2_buffers.c | 216
> > --
> > libavcodec/v4l2_buffers.h | 4 +
> > libavcodec/v4l2_context.c | 40 ++-
> > libavcodec/v4l2_m2m.c | 4 +-
> > libavcodec/v4l2_m2m.h | 3 +
> > libavcodec/v4l2_m2m_dec.c | 23
> > 6 files changed, 253 insertions(+), 37 deletions(-)
>
> The v4l2_m2m decoders need to depend on libdrm in configure for
> this. (And if you don't want that as a hard dependency then you'll
> need #ifdefs everywhere.)
Yes, I'll update the patch to include libdrm
>
> > diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> > index aef911f3bb..e5c46ac81e 100644
> > --- a/libavcodec/v4l2_buffers.c
> > +++ b/libavcodec/v4l2_buffers.c
> > @@ -21,6 +21,7 @@
> > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301 USA
> > */
> >
> > +#include
>
> Don't include the outer path, pkg-config deals with it. (The path
> you've picked is wrong for a default install of current libdrm.)
>
I'll fix that. Thanks!
> > #include
> > #include
> > #include
> > @@ -29,6 +30,7 @@
> > #include
> > #include "libavcodec/avcodec.h"
> > #include "libavcodec/internal.h"
> > +#include "libavutil/hwcontext.h"
> > #include "v4l2_context.h"
> > #include "v4l2_buffers.h"
> > #include "v4l2_m2m.h"
> > @@ -203,7 +205,79 @@ static enum AVColorTransferCharacteristic
> > v4l2_get_color_trc(V4L2Buffer *buf)
> > return AVCOL_TRC_UNSPECIFIED;
> > }
> >
> > -static void v4l2_free_buffer(void *opaque, uint8_t *unused)
> > +static uint8_t * v4l2_get_drm_frame(V4L2Buffer *avbuf)
> > +{
> > +AVDRMFrameDescriptor *drm_desc = >drm_frame;
> > +AVDRMLayerDescriptor *layer;
> > +
> > +/* fill the DRM frame descriptor */
> > +drm_desc->nb_objects = avbuf->num_planes;
> > +drm_desc->nb_layers = 1;
> > +
> > +layer = _desc->layers[0];
> > +layer->nb_planes = avbuf->num_planes;
> > +
> > +for (int i = 0; i < avbuf->num_planes; i++) {
> > +layer->planes[i].object_index = i;
> > +layer->planes[i].offset = 0;
> > +layer->planes[i].pitch = avbuf-
> > >plane_info[i].bytesperline;
> > +}
> > +
> > +switch (avbuf->context->av_pix_fmt) {
> > +case AV_PIX_FMT_YUYV422:
> > +
> > +layer->format = DRM_FORMAT_YUYV;
> > +layer->nb_planes = 1;
> > +
> > +break;
> > +
> > +case AV_PIX_FMT_NV12:
> > +case AV_PIX_FMT_NV21:
> > +
> > +layer->format = avbuf->context->av_pix_fmt ==
> > AV_PIX_FMT_NV12 ?
> > +DRM_FORMAT_NV12 : DRM_FORMAT_NV21;
> > +
> > +if (avbuf->num_planes > 1)
> > +break;
> > +
> > +layer->nb_planes = 2;
> > +
> > +layer->planes[1].object_index = 0;
> > +layer->planes[1].offset = avbuf-
> > >plane_info[0].bytesperline *
> > +avbuf->context->format.fmt.pix.height;
> > +layer->planes[1].pitch = avbuf-
> > >plane_info[0].bytesperline;
>
> To confirm, it's necessarily true that there is no padding at all
> between the luma and chroma planes here?
>
I've never seen any padding on the three devices I have that can output
NV12.
> > +break;
> > +
> > +case AV_PIX_FMT_YUV420P:
> > +
> > +layer->format = DRM_FORMAT_YUV420;
> > +
> > +if (avbuf->num_planes > 1)
> > +break;
> > +
> > +layer->nb_planes = 3;
> > +
> > +layer->planes[1].object_index = 0;
> > +layer->planes[1].offset = avbuf-
> > >plane_info[0].bytesperline *
> > +avbuf->context->format.fmt.pix.height;
> > +layer->planes[1].pitch = avbuf->plane_info[0].bytesperline
> > >> 1;
> > +
> > +layer->planes[2].object_index = 0;
> > +layer->planes[2].offset = layer->planes[1].offset +
> > +((avbuf->plane_info[0].bytesperline *
> > + avbuf->context->format.fmt.pix.height) >> 2);
> > +
Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
On 12 August 2018 at 22:25, Jorge Ramirez-Ortiz wrote:
> On 08/06/2018 10:12 PM, Mark Thompson wrote:
>>
>> On 06/08/18 16:44, Jorge Ramirez-Ortiz wrote:
>>>
>>> On 08/04/2018 11:43 PM, Mark Thompson wrote:
>
> diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c
> index efcb0426e4..9457fadb1e 100644
> --- a/libavcodec/v4l2_context.c
> +++ b/libavcodec/v4l2_context.c
> @@ -393,22 +393,54 @@ static int v4l2_release_buffers(V4L2Context* ctx)
>struct v4l2_requestbuffers req = {
>.memory = V4L2_MEMORY_MMAP,
>.type = ctx->type,
> -.count = 0, /* 0 -> unmaps buffers from the driver */
> +.count = 0, /* 0 -> unmap all buffers from the driver */
>};
> -int i, j;
> +int ret, i, j;
> for (i = 0; i < ctx->num_buffers; i++) {
>V4L2Buffer *buffer = >buffers[i];
> for (j = 0; j < buffer->num_planes; j++) {
>struct V4L2Plane_info *p = >plane_info[j];
> +
> +if (V4L2_TYPE_IS_OUTPUT(ctx->type)) {
> +/* output buffers are not EXPORTED */
> +goto unmap;
> +}
> +
> +if (ctx_to_m2mctx(ctx)->output_drm) {
> +/* use the DRM frame to close */
> +if (buffer->drm_frame.objects[j].fd >= 0) {
> +if (close(buffer->drm_frame.objects[j].fd) < 0) {
> +av_log(logger(ctx), AV_LOG_ERROR, "%s close
> drm fd "
> +"[buffer=%2d, plane=%d, fd=%2d] - %s \n",
> +ctx->name, i, j,
> buffer->drm_frame.objects[j].fd,
> +av_err2str(AVERROR(errno)));
> +}
> +}
> +}
> +unmap:
>if (p->mm_addr && p->length)
>if (munmap(p->mm_addr, p->length) < 0)
> -av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane
> (%s))\n", ctx->name, av_err2str(AVERROR(errno)));
> +av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane
> (%s))\n",
> +ctx->name, av_err2str(AVERROR(errno)));
>}
(This whole function feels like it might fit better in v4l2_buffers.c?)
To check my understanding here of what you've got currently (please
correct me if I make a mistake here):
* When making a new set of buffers (on start or format change),
VIDIOC_EXPBUF is called once for each V4L2 buffer to make a DRM PRIME fd
for
it.
* Whenever you want to return a buffer, return the fd instead if using
DRM PRIME output.
* When returning a set of buffers (on close or format change), wait for
all references to disappear and then close all of the fds before releasing
the V4L2 buffers.
>>>
>>> We kept it as a context operation since release_buffers is not a per
>>> buffer operation (it just an operation that applies on all buffers, like all
>>> context operations IIRC ).
>>> The problem is that even if we close the file descriptors when all
>>> references have gone, the client might still have GEM objects associated so
>>> we would fail at releasing the buffers.
>>
>> Ok.
>>
>>> This was noticed during testing (fixed in the test code with this commit)
>>> [1]
>>> [1]
>>> https://github.com/BayLibre/ffmpeg-drm/commit/714288ab9d86397dd8230068fd9a7d3d4d76b802
>>>
>>> And a reminder was added to ffmpeg below
>>>
>}
>-return ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
> +ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
> +if (ret < 0) {
> +av_log(logger(ctx), AV_LOG_ERROR, "release all %s buffers
> (%s)\n",
> +ctx->name, av_err2str(AVERROR(errno)));
> +
> +if (ctx_to_m2mctx(ctx)->output_drm)
> +av_log(logger(ctx), AV_LOG_ERROR,
> +"Make sure the DRM client releases all FB/GEM
> objects before closing the codec (ie):\n"
> +"for all buffers: \n"
> +" 1. drmModeRmFB(..)\n"
> +" 2. drmIoctl(.., DRM_IOCTL_GEM_CLOSE,... )\n");
Is it possible to hit this case? Waiting for all references to
disappear seems like it should cover it. (And if the user has freed an
object they are still using then that's certainly undefined behaviour, so
if
that's the only case here it would probably be better to abort() so that
it's caught immediately.)
>>> yes (as per the above explanation)
>>
>> Does errno indicate that we've hit this case specifically rather than e.g.
>> some sort of hardware problem (decoder device physically disconnected or
>> whatever)?
>
>
> it just returns the standard "Device or resource busy"
Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
On 08/06/2018 10:12 PM, Mark Thompson wrote:
On 06/08/18 16:44, Jorge Ramirez-Ortiz wrote:
On 08/04/2018 11:43 PM, Mark Thompson wrote:
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c
index efcb0426e4..9457fadb1e 100644
--- a/libavcodec/v4l2_context.c
+++ b/libavcodec/v4l2_context.c
@@ -393,22 +393,54 @@ static int v4l2_release_buffers(V4L2Context* ctx)
struct v4l2_requestbuffers req = {
.memory = V4L2_MEMORY_MMAP,
.type = ctx->type,
- .count = 0, /* 0 -> unmaps buffers from the driver */
+ .count = 0, /* 0 -> unmap all buffers from the driver */
};
- int i, j;
+ int ret, i, j;
for (i = 0; i < ctx->num_buffers; i++) {
V4L2Buffer *buffer = >buffers[i];
for (j = 0; j < buffer->num_planes; j++) {
struct V4L2Plane_info *p = >plane_info[j];
+
+ if (V4L2_TYPE_IS_OUTPUT(ctx->type)) {
+ /* output buffers are not EXPORTED */
+ goto unmap;
+ }
+
+ if (ctx_to_m2mctx(ctx)->output_drm) {
+ /* use the DRM frame to close */
+ if (buffer->drm_frame.objects[j].fd >= 0) {
+ if (close(buffer->drm_frame.objects[j].fd) < 0) {
+ av_log(logger(ctx), AV_LOG_ERROR, "%s close drm fd "
+ "[buffer=%2d, plane=%d, fd=%2d] - %s \n",
+ ctx->name, i, j, buffer->drm_frame.objects[j].fd,
+ av_err2str(AVERROR(errno)));
+ }
+ }
+ }
+unmap:
if (p->mm_addr && p->length)
if (munmap(p->mm_addr, p->length) < 0)
- av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane (%s))\n",
ctx->name, av_err2str(AVERROR(errno)));
+ av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane (%s))\n",
+ ctx->name, av_err2str(AVERROR(errno)));
}
(This whole function feels like it might fit better in v4l2_buffers.c?)
To check my understanding here of what you've got currently (please correct me
if I make a mistake here):
* When making a new set of buffers (on start or format change), VIDIOC_EXPBUF
is called once for each V4L2 buffer to make a DRM PRIME fd for it.
* Whenever you want to return a buffer, return the fd instead if using DRM
PRIME output.
* When returning a set of buffers (on close or format change), wait for all
references to disappear and then close all of the fds before releasing the V4L2
buffers.
We kept it as a context operation since release_buffers is not a per buffer
operation (it just an operation that applies on all buffers, like all context
operations IIRC ).
The problem is that even if we close the file descriptors when all references
have gone, the client might still have GEM objects associated so we would fail
at releasing the buffers.
Ok.
This was noticed during testing (fixed in the test code with this commit) [1]
[1]
https://github.com/BayLibre/ffmpeg-drm/commit/714288ab9d86397dd8230068fd9a7d3d4d76b802
And a reminder was added to ffmpeg below
}
- return ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
+ ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
+ if (ret < 0) {
+ av_log(logger(ctx), AV_LOG_ERROR, "release all %s buffers (%s)\n",
+ ctx->name, av_err2str(AVERROR(errno)));
+
+ if (ctx_to_m2mctx(ctx)->output_drm)
+ av_log(logger(ctx), AV_LOG_ERROR,
+ "Make sure the DRM client releases all FB/GEM objects before
closing the codec (ie):\n"
+ "for all buffers: \n"
+ " 1. drmModeRmFB(..)\n"
+ " 2. drmIoctl(.., DRM_IOCTL_GEM_CLOSE,... )\n");
Is it possible to hit this case? Waiting for all references to disappear seems
like it should cover it. (And if the user has freed an object they are still
using then that's certainly undefined behaviour, so if that's the only case
here it would probably be better to abort() so that it's caught immediately.)
yes (as per the above explanation)
Does errno indicate that we've hit this case specifically rather than e.g. some
sort of hardware problem (decoder device physically disconnected or whatever)?
it just returns the standard "Device or resource busy" when we try to
release the buffers since they are still in use
Since it's a use-after-free on the part of the API user and therefore undefined
behaviour, we should av_assert0() that it doesn't happen if we can identify it.
yes I agree.
should we assert on top of the error message or better get rid of the
message and just add a comment to the code?
(The KMS/GEM comments would also be rather confusing if the sink is something
else - GL/EGL seems likely to be common, and OpenCL or Vulkan are certainly
possible too. Maybe make that more generic.)
- Mark
Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
On 06/08/18 16:44, Jorge Ramirez-Ortiz wrote:
> On 08/04/2018 11:43 PM, Mark Thompson wrote:
>>> diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c
>>> index efcb0426e4..9457fadb1e 100644
>>> --- a/libavcodec/v4l2_context.c
>>> +++ b/libavcodec/v4l2_context.c
>>> @@ -393,22 +393,54 @@ static int v4l2_release_buffers(V4L2Context* ctx)
>>> struct v4l2_requestbuffers req = {
>>> .memory = V4L2_MEMORY_MMAP,
>>> .type = ctx->type,
>>> - .count = 0, /* 0 -> unmaps buffers from the driver */
>>> + .count = 0, /* 0 -> unmap all buffers from the driver */
>>> };
>>> - int i, j;
>>> + int ret, i, j;
>>> for (i = 0; i < ctx->num_buffers; i++) {
>>> V4L2Buffer *buffer = >buffers[i];
>>> for (j = 0; j < buffer->num_planes; j++) {
>>> struct V4L2Plane_info *p = >plane_info[j];
>>> +
>>> + if (V4L2_TYPE_IS_OUTPUT(ctx->type)) {
>>> + /* output buffers are not EXPORTED */
>>> + goto unmap;
>>> + }
>>> +
>>> + if (ctx_to_m2mctx(ctx)->output_drm) {
>>> + /* use the DRM frame to close */
>>> + if (buffer->drm_frame.objects[j].fd >= 0) {
>>> + if (close(buffer->drm_frame.objects[j].fd) < 0) {
>>> + av_log(logger(ctx), AV_LOG_ERROR, "%s close drm fd
>>> "
>>> + "[buffer=%2d, plane=%d, fd=%2d] - %s \n",
>>> + ctx->name, i, j,
>>> buffer->drm_frame.objects[j].fd,
>>> + av_err2str(AVERROR(errno)));
>>> + }
>>> + }
>>> + }
>>> +unmap:
>>> if (p->mm_addr && p->length)
>>> if (munmap(p->mm_addr, p->length) < 0)
>>> - av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane
>>> (%s))\n", ctx->name, av_err2str(AVERROR(errno)));
>>> + av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane
>>> (%s))\n",
>>> + ctx->name, av_err2str(AVERROR(errno)));
>>> }
>> (This whole function feels like it might fit better in v4l2_buffers.c?)
>>
>> To check my understanding here of what you've got currently (please correct
>> me if I make a mistake here):
>> * When making a new set of buffers (on start or format change),
>> VIDIOC_EXPBUF is called once for each V4L2 buffer to make a DRM PRIME fd for
>> it.
>> * Whenever you want to return a buffer, return the fd instead if using DRM
>> PRIME output.
>> * When returning a set of buffers (on close or format change), wait for all
>> references to disappear and then close all of the fds before releasing the
>> V4L2 buffers.
>
> We kept it as a context operation since release_buffers is not a per buffer
> operation (it just an operation that applies on all buffers, like all context
> operations IIRC ).
> The problem is that even if we close the file descriptors when all references
> have gone, the client might still have GEM objects associated so we would
> fail at releasing the buffers.
Ok.
> This was noticed during testing (fixed in the test code with this commit) [1]
> [1]
> https://github.com/BayLibre/ffmpeg-drm/commit/714288ab9d86397dd8230068fd9a7d3d4d76b802
>
> And a reminder was added to ffmpeg below
>
>>> }
>>> - return ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
>>> + ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
>>> + if (ret < 0) {
>>> + av_log(logger(ctx), AV_LOG_ERROR, "release all %s buffers
>>> (%s)\n",
>>> + ctx->name, av_err2str(AVERROR(errno)));
>>> +
>>> + if (ctx_to_m2mctx(ctx)->output_drm)
>>> + av_log(logger(ctx), AV_LOG_ERROR,
>>> + "Make sure the DRM client releases all FB/GEM objects
>>> before closing the codec (ie):\n"
>>> + "for all buffers: \n"
>>> + " 1. drmModeRmFB(..)\n"
>>> + " 2. drmIoctl(.., DRM_IOCTL_GEM_CLOSE,... )\n");
>> Is it possible to hit this case? Waiting for all references to disappear
>> seems like it should cover it. (And if the user has freed an object they
>> are still using then that's certainly undefined behaviour, so if that's the
>> only case here it would probably be better to abort() so that it's caught
>> immediately.)
>>
>
> yes (as per the above explanation)
Does errno indicate that we've hit this case specifically rather than e.g. some
sort of hardware problem (decoder device physically disconnected or whatever)?
Since it's a use-after-free on the part of the API user and therefore undefined
behaviour, we should av_assert0() that it doesn't happen if we can identify it.
(The KMS/GEM comments would also be rather confusing if the sink is something
else - GL/EGL seems likely to be common, and OpenCL or Vulkan are certainly
possible too. Maybe make that more generic.)
- Mark
Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
On 08/04/2018 11:43 PM, Mark Thompson wrote:
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c
index efcb0426e4..9457fadb1e 100644
--- a/libavcodec/v4l2_context.c
+++ b/libavcodec/v4l2_context.c
@@ -393,22 +393,54 @@ static int v4l2_release_buffers(V4L2Context* ctx)
struct v4l2_requestbuffers req = {
.memory = V4L2_MEMORY_MMAP,
.type = ctx->type,
-.count = 0, /* 0 -> unmaps buffers from the driver */
+.count = 0, /* 0 -> unmap all buffers from the driver */
};
-int i, j;
+int ret, i, j;
for (i = 0; i < ctx->num_buffers; i++) {
V4L2Buffer *buffer = >buffers[i];
for (j = 0; j < buffer->num_planes; j++) {
struct V4L2Plane_info *p = >plane_info[j];
+
+if (V4L2_TYPE_IS_OUTPUT(ctx->type)) {
+/* output buffers are not EXPORTED */
+goto unmap;
+}
+
+if (ctx_to_m2mctx(ctx)->output_drm) {
+/* use the DRM frame to close */
+if (buffer->drm_frame.objects[j].fd >= 0) {
+if (close(buffer->drm_frame.objects[j].fd) < 0) {
+av_log(logger(ctx), AV_LOG_ERROR, "%s close drm fd "
+"[buffer=%2d, plane=%d, fd=%2d] - %s \n",
+ctx->name, i, j, buffer->drm_frame.objects[j].fd,
+av_err2str(AVERROR(errno)));
+}
+}
+}
+unmap:
if (p->mm_addr && p->length)
if (munmap(p->mm_addr, p->length) < 0)
-av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane (%s))\n",
ctx->name, av_err2str(AVERROR(errno)));
+av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane (%s))\n",
+ctx->name, av_err2str(AVERROR(errno)));
}
(This whole function feels like it might fit better in v4l2_buffers.c?)
To check my understanding here of what you've got currently (please correct me
if I make a mistake here):
* When making a new set of buffers (on start or format change), VIDIOC_EXPBUF
is called once for each V4L2 buffer to make a DRM PRIME fd for it.
* Whenever you want to return a buffer, return the fd instead if using DRM
PRIME output.
* When returning a set of buffers (on close or format change), wait for all
references to disappear and then close all of the fds before releasing the V4L2
buffers.
We kept it as a context operation since release_buffers is not a per
buffer operation (it just an operation that applies on all buffers, like
all context operations IIRC ).
The problem is that even if we close the file descriptors when all
references have gone, the client might still have GEM objects associated
so we would fail at releasing the buffers.
This was noticed during testing (fixed in the test code with this
commit) [1]
[1]
https://github.com/BayLibre/ffmpeg-drm/commit/714288ab9d86397dd8230068fd9a7d3d4d76b802
And a reminder was added to ffmpeg below
}
-return ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
+ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, );
+if (ret < 0) {
+av_log(logger(ctx), AV_LOG_ERROR, "release all %s buffers (%s)\n",
+ctx->name, av_err2str(AVERROR(errno)));
+
+if (ctx_to_m2mctx(ctx)->output_drm)
+av_log(logger(ctx), AV_LOG_ERROR,
+"Make sure the DRM client releases all FB/GEM objects before
closing the codec (ie):\n"
+"for all buffers: \n"
+" 1. drmModeRmFB(..)\n"
+" 2. drmIoctl(.., DRM_IOCTL_GEM_CLOSE,... )\n");
Is it possible to hit this case? Waiting for all references to disappear seems
like it should cover it. (And if the user has freed an object they are still
using then that's certainly undefined behaviour, so if that's the only case
here it would probably be better to abort() so that it's caught immediately.)
yes (as per the above explanation)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
On 04/08/18 01:40, Lukas Rusak wrote:
> This allows for a zero-copy output by exporting the v4l2 buffer then wrapping
> that buffer
> in the AVDRMFrameDescriptor like it is done in rkmpp.
>
> This has been in use for quite some time with great success on many platforms
> including:
> - Amlogic S905
> - Raspberry Pi
> - i.MX6
> - Dragonboard 410c
>
> This was developed in conjunction with Kodi to allow handling the zero-copy
> buffer rendering.
> A simply utility for testing is also available here:
> https://github.com/BayLibre/ffmpeg-drm
>
> todo:
> - allow selecting pixel format output from decoder
> - allow configuring amount of output and capture buffers
>
> V2:
> - allow selecting AV_PIX_FMT_DRM_PRIME
>
> V3:
> - use get_format to select AV_PIX_FMT_DRM_PRIME
> - use hw_configs
> - add handling of AV_PIX_FMT_YUV420P format (for raspberry pi)
> - add handling of AV_PIX_FMT_YUYV422 format (for i.MX6 coda decoder)
> ---
> libavcodec/v4l2_buffers.c | 216 --
> libavcodec/v4l2_buffers.h | 4 +
> libavcodec/v4l2_context.c | 40 ++-
> libavcodec/v4l2_m2m.c | 4 +-
> libavcodec/v4l2_m2m.h | 3 +
> libavcodec/v4l2_m2m_dec.c | 23
> 6 files changed, 253 insertions(+), 37 deletions(-)
The v4l2_m2m decoders need to depend on libdrm in configure for this. (And if
you don't want that as a hard dependency then you'll need #ifdefs everywhere.)
> diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> index aef911f3bb..e5c46ac81e 100644
> --- a/libavcodec/v4l2_buffers.c
> +++ b/libavcodec/v4l2_buffers.c
> @@ -21,6 +21,7 @@
> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> */
>
> +#include
Don't include the outer path, pkg-config deals with it. (The path you've
picked is wrong for a default install of current libdrm.)
> #include
> #include
> #include
> @@ -29,6 +30,7 @@
> #include
> #include "libavcodec/avcodec.h"
> #include "libavcodec/internal.h"
> +#include "libavutil/hwcontext.h"
> #include "v4l2_context.h"
> #include "v4l2_buffers.h"
> #include "v4l2_m2m.h"
> @@ -203,7 +205,79 @@ static enum AVColorTransferCharacteristic
> v4l2_get_color_trc(V4L2Buffer *buf)
> return AVCOL_TRC_UNSPECIFIED;
> }
>
> -static void v4l2_free_buffer(void *opaque, uint8_t *unused)
> +static uint8_t * v4l2_get_drm_frame(V4L2Buffer *avbuf)
> +{
> +AVDRMFrameDescriptor *drm_desc = >drm_frame;
> +AVDRMLayerDescriptor *layer;
> +
> +/* fill the DRM frame descriptor */
> +drm_desc->nb_objects = avbuf->num_planes;
> +drm_desc->nb_layers = 1;
> +
> +layer = _desc->layers[0];
> +layer->nb_planes = avbuf->num_planes;
> +
> +for (int i = 0; i < avbuf->num_planes; i++) {
> +layer->planes[i].object_index = i;
> +layer->planes[i].offset = 0;
> +layer->planes[i].pitch = avbuf->plane_info[i].bytesperline;
> +}
> +
> +switch (avbuf->context->av_pix_fmt) {
> +case AV_PIX_FMT_YUYV422:
> +
> +layer->format = DRM_FORMAT_YUYV;
> +layer->nb_planes = 1;
> +
> +break;
> +
> +case AV_PIX_FMT_NV12:
> +case AV_PIX_FMT_NV21:
> +
> +layer->format = avbuf->context->av_pix_fmt == AV_PIX_FMT_NV12 ?
> +DRM_FORMAT_NV12 : DRM_FORMAT_NV21;
> +
> +if (avbuf->num_planes > 1)
> +break;
> +
> +layer->nb_planes = 2;
> +
> +layer->planes[1].object_index = 0;
> +layer->planes[1].offset = avbuf->plane_info[0].bytesperline *
> +avbuf->context->format.fmt.pix.height;
> +layer->planes[1].pitch = avbuf->plane_info[0].bytesperline;
To confirm, it's necessarily true that there is no padding at all between the
luma and chroma planes here?
> +break;
> +
> +case AV_PIX_FMT_YUV420P:
> +
> +layer->format = DRM_FORMAT_YUV420;
> +
> +if (avbuf->num_planes > 1)
> +break;
> +
> +layer->nb_planes = 3;
> +
> +layer->planes[1].object_index = 0;
> +layer->planes[1].offset = avbuf->plane_info[0].bytesperline *
> +avbuf->context->format.fmt.pix.height;
> +layer->planes[1].pitch = avbuf->plane_info[0].bytesperline >> 1;
> +
> +layer->planes[2].object_index = 0;
> +layer->planes[2].offset = layer->planes[1].offset +
> +((avbuf->plane_info[0].bytesperline *
> + avbuf->context->format.fmt.pix.height) >> 2);
> +layer->planes[2].pitch = avbuf->plane_info[0].bytesperline >> 1;
> +break;
> +
> +default:
> +drm_desc->nb_layers = 0;
> +break;
> +}
> +
> +return (uint8_t *) drm_desc;
> +}
> +
> +static void v4l2_free_buffer(void *opaque, uint8_t *data)
> {
> V4L2Buffer* avbuf = opaque;
> V4L2m2mContext *s = buf_to_m2mctx(avbuf);
> @@ -227,27 +301,47 @@ static void v4l2_free_buffer(void *opaque, uint8_t
> *unused)
> }
> }
>
> -static int v4l2_buf_to_bufref(V4L2Buffer *in, int
[FFmpeg-devel] [PATCH 2/4] libavcodec: v4l2m2m: output AVDRMFrameDescriptor
This allows for a zero-copy output by exporting the v4l2 buffer then wrapping
that buffer
in the AVDRMFrameDescriptor like it is done in rkmpp.
This has been in use for quite some time with great success on many platforms
including:
- Amlogic S905
- Raspberry Pi
- i.MX6
- Dragonboard 410c
This was developed in conjunction with Kodi to allow handling the zero-copy
buffer rendering.
A simply utility for testing is also available here:
https://github.com/BayLibre/ffmpeg-drm
todo:
- allow selecting pixel format output from decoder
- allow configuring amount of output and capture buffers
V2:
- allow selecting AV_PIX_FMT_DRM_PRIME
V3:
- use get_format to select AV_PIX_FMT_DRM_PRIME
- use hw_configs
- add handling of AV_PIX_FMT_YUV420P format (for raspberry pi)
- add handling of AV_PIX_FMT_YUYV422 format (for i.MX6 coda decoder)
---
libavcodec/v4l2_buffers.c | 216 --
libavcodec/v4l2_buffers.h | 4 +
libavcodec/v4l2_context.c | 40 ++-
libavcodec/v4l2_m2m.c | 4 +-
libavcodec/v4l2_m2m.h | 3 +
libavcodec/v4l2_m2m_dec.c | 23
6 files changed, 253 insertions(+), 37 deletions(-)
diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
index aef911f3bb..e5c46ac81e 100644
--- a/libavcodec/v4l2_buffers.c
+++ b/libavcodec/v4l2_buffers.c
@@ -21,6 +21,7 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*/
+#include
#include
#include
#include
@@ -29,6 +30,7 @@
#include
#include "libavcodec/avcodec.h"
#include "libavcodec/internal.h"
+#include "libavutil/hwcontext.h"
#include "v4l2_context.h"
#include "v4l2_buffers.h"
#include "v4l2_m2m.h"
@@ -203,7 +205,79 @@ static enum AVColorTransferCharacteristic
v4l2_get_color_trc(V4L2Buffer *buf)
return AVCOL_TRC_UNSPECIFIED;
}
-static void v4l2_free_buffer(void *opaque, uint8_t *unused)
+static uint8_t * v4l2_get_drm_frame(V4L2Buffer *avbuf)
+{
+AVDRMFrameDescriptor *drm_desc = >drm_frame;
+AVDRMLayerDescriptor *layer;
+
+/* fill the DRM frame descriptor */
+drm_desc->nb_objects = avbuf->num_planes;
+drm_desc->nb_layers = 1;
+
+layer = _desc->layers[0];
+layer->nb_planes = avbuf->num_planes;
+
+for (int i = 0; i < avbuf->num_planes; i++) {
+layer->planes[i].object_index = i;
+layer->planes[i].offset = 0;
+layer->planes[i].pitch = avbuf->plane_info[i].bytesperline;
+}
+
+switch (avbuf->context->av_pix_fmt) {
+case AV_PIX_FMT_YUYV422:
+
+layer->format = DRM_FORMAT_YUYV;
+layer->nb_planes = 1;
+
+break;
+
+case AV_PIX_FMT_NV12:
+case AV_PIX_FMT_NV21:
+
+layer->format = avbuf->context->av_pix_fmt == AV_PIX_FMT_NV12 ?
+DRM_FORMAT_NV12 : DRM_FORMAT_NV21;
+
+if (avbuf->num_planes > 1)
+break;
+
+layer->nb_planes = 2;
+
+layer->planes[1].object_index = 0;
+layer->planes[1].offset = avbuf->plane_info[0].bytesperline *
+avbuf->context->format.fmt.pix.height;
+layer->planes[1].pitch = avbuf->plane_info[0].bytesperline;
+break;
+
+case AV_PIX_FMT_YUV420P:
+
+layer->format = DRM_FORMAT_YUV420;
+
+if (avbuf->num_planes > 1)
+break;
+
+layer->nb_planes = 3;
+
+layer->planes[1].object_index = 0;
+layer->planes[1].offset = avbuf->plane_info[0].bytesperline *
+avbuf->context->format.fmt.pix.height;
+layer->planes[1].pitch = avbuf->plane_info[0].bytesperline >> 1;
+
+layer->planes[2].object_index = 0;
+layer->planes[2].offset = layer->planes[1].offset +
+((avbuf->plane_info[0].bytesperline *
+ avbuf->context->format.fmt.pix.height) >> 2);
+layer->planes[2].pitch = avbuf->plane_info[0].bytesperline >> 1;
+break;
+
+default:
+drm_desc->nb_layers = 0;
+break;
+}
+
+return (uint8_t *) drm_desc;
+}
+
+static void v4l2_free_buffer(void *opaque, uint8_t *data)
{
V4L2Buffer* avbuf = opaque;
V4L2m2mContext *s = buf_to_m2mctx(avbuf);
@@ -227,27 +301,47 @@ static void v4l2_free_buffer(void *opaque, uint8_t
*unused)
}
}
-static int v4l2_buf_to_bufref(V4L2Buffer *in, int plane, AVBufferRef **buf)
+static int v4l2_buffer_export_drm(V4L2Buffer* avbuf)
{
-V4L2m2mContext *s = buf_to_m2mctx(in);
+struct v4l2_exportbuffer expbuf;
+int i, ret;
-if (plane >= in->num_planes)
-return AVERROR(EINVAL);
+for (i = 0; i < avbuf->num_planes; i++) {
+memset(, 0, sizeof(expbuf));
-/* even though most encoders return 0 in data_offset encoding vp8 does
require this value */
-*buf = av_buffer_create((char *)in->plane_info[plane].mm_addr +
in->planes[plane].data_offset,
-in->plane_info[plane].length, v4l2_free_buffer,
in, 0);
-if (!*buf)
-return AVERROR(ENOMEM);
+expbuf.index = avbuf->buf.index;
+expbuf.type =
