On 17/10/17 18:20, Jorge Ramirez-Ortiz wrote:
> A user can close the codec while keeping references to some of the
> capture buffers. When the codec is closed, the structure that keeps
> the contexts, state and the driver file descriptor is freed.
>
> Since access to some of the elements in that structure is required to
> properly release the memory during the buffer unref callbacks, the
> following commit makes sure the unref callback accesses valid memory.
>
> This commit was tested with valgrind:
>
> $ valgrind ffmpeg_g -report -threads 1 -v 55 -y -c:v h264_v4l2m2m \
> -i video.h264 -an -frames:v 100 -f null -
> ---
> libavcodec/v4l2_buffers.c | 17 ++++++++++++++++-
> libavcodec/v4l2_buffers.h | 6 ++++++
> libavcodec/v4l2_m2m.c | 29 ++++++++++++++++++++++++-----
> libavcodec/v4l2_m2m_dec.c | 2 +-
> 4 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> index ba70c5d..80e65ca 100644
> --- a/libavcodec/v4l2_buffers.c
> +++ b/libavcodec/v4l2_buffers.c
> @@ -205,9 +205,24 @@ static enum AVColorTransferCharacteristic
> v4l2_get_color_trc(V4L2Buffer *buf)
> static void v4l2_free_buffer(void *opaque, uint8_t *unused)
> {
> V4L2Buffer* avbuf = opaque;
> - V4L2m2mContext *s = buf_to_m2mctx(avbuf);
> + V4L2m2mContext *s = avbuf->m2m ? avbuf->m2m : buf_to_m2mctx(avbuf);
>
> atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel);
> +
> + if (avbuf->m2m) {
> + if (!atomic_load(&s->refcount)) {
> + /* unmmap and free the associated buffers */
> + ff_v4l2_context_release(&s->capture);
> +
> + /* close the v4l2 driver */
> + close(s->fd);
> +
> + /* release the duplicated m2m context */
> + av_freep(&s);
> + }
> + return;
> + }
> +
> if (s->reinit) {
> if (!atomic_load(&s->refcount))
> sem_post(&s->refsync);
> diff --git a/libavcodec/v4l2_buffers.h b/libavcodec/v4l2_buffers.h
> index e28a4a6..d774480 100644
> --- a/libavcodec/v4l2_buffers.h
> +++ b/libavcodec/v4l2_buffers.h
> @@ -41,6 +41,12 @@ typedef struct V4L2Buffer {
> /* each buffer needs to have a reference to its context */
> struct V4L2Context *context;
>
> + /* when the codec is closed while the user has buffer references we
> + * still need access to context data (this is a pointer to a duplicated
> + * m2m context created during the codec close function).
> + */
> + struct V4L2m2mContext *m2m;
> +
> /* keep track of the mmap address and mmap length */
> struct V4L2Plane_info {
> int bytesperline;
> diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
> index 1d7a852..91b1bbb 100644
> --- a/libavcodec/v4l2_m2m.c
> +++ b/libavcodec/v4l2_m2m.c
> @@ -313,8 +313,8 @@ error:
>
> int ff_v4l2_m2m_codec_end(AVCodecContext *avctx)
> {
> - V4L2m2mContext* s = avctx->priv_data;
> - int ret;
> + V4L2m2mContext *m2m, *s = avctx->priv_data;
> + int i, ret;
>
> ret = ff_v4l2_context_set_status(&s->output, VIDIOC_STREAMOFF);
> if (ret)
> @@ -325,12 +325,31 @@ int ff_v4l2_m2m_codec_end(AVCodecContext *avctx)
> av_log(avctx, AV_LOG_ERROR, "VIDIOC_STREAMOFF %s\n",
> s->capture.name);
>
> ff_v4l2_context_release(&s->output);
> + sem_destroy(&s->refsync);
>
> - if (atomic_load(&s->refcount))
> - av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end leaving pending
> buffers\n");
> + if (atomic_load(&s->refcount)) {
> + av_log(avctx, AV_LOG_DEBUG, "ff_v4l2m2m_codec_end, referenced
> buffers %d\n", atomic_load(&s->refcount));
> +
> + /* We are about to free the private data while the user still has
> references to the buffers.
> + * This means that when the user releases those buffers, the
> v4l2_free_buffer callback will be pointing to free'd memory.
> + * Duplicate the m2m context and update the buffers.
> + */
> + m2m = av_mallocz(sizeof(*m2m));
> + if (m2m) {
> + memcpy(m2m, s, sizeof(V4L2m2mContext));
> + for (i = 0; i < s->capture.num_buffers; i++)
> + s->capture.buffers[i].m2m = m2m;
> +
> + return 0;
> + }
No.
The user can call av_buffer_unref() and therefore v4l2_free_buffer()
asynchronously on another thread while this manoeuvre is in progress.
> +
> + /* on ENOMEM let's at least close the v4l2 driver and release the
> capture memory
> + * so the driver is left in a healthy state.
> + */
> + av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end, referenced
> buffers not accessible\n");
> + }
>
> ff_v4l2_context_release(&s->capture);
> - sem_destroy(&s->refsync);
>
> /* release the hardware */
> if (close(s->fd) < 0 )
> diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
> index 958cdc5..f88f819 100644
> --- a/libavcodec/v4l2_m2m_dec.c
> +++ b/libavcodec/v4l2_m2m_dec.c
> @@ -38,7 +38,7 @@ static int v4l2_try_start(AVCodecContext *avctx)
> V4L2m2mContext *s = avctx->priv_data;
> V4L2Context *const capture = &s->capture;
> V4L2Context *const output = &s->output;
> - struct v4l2_selection selection;
> + struct v4l2_selection selection = { 0 };
This looks like it should be a separate change.
> int ret;
>
> /* 1. start the output process */
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
