On 19/10/17 18:55, Jorge Ramirez wrote:
> On 10/19/2017 11:49 AM, Mark Thompson wrote:
>> On 19/10/17 08:28, Jorge Ramirez wrote:
>>> On 10/19/2017 02:10 AM, Mark Thompson wrote:
>>>> Refcount all of the context information.
>>>> ---
>>>> As discussed in the other thread, something like this. We move most of
>>>> the context into a refcounted buffer and AVCodecContext.priv_data is left
>>>> as a stub holding a reference to it.
>>> um, ok ... please could you send a patch so I can apply it? I see several
>>> problems in v4l2_free_buffer.
>> What goes wrong? It applies fine for me on current head
>> (f4090940bd3024e69d236257d327f11d1e496229).
>
> yes my bad.
>
>>
>>> To sum up the RFC, instead of using private_data to place the codec's
>>> context, it uses private data to place a _pointer_ to an allocated codec
>>> context "just" so it wont be deallocated after the codec is closed (codec
>>> close frees the private_data)
>>>
>>> Personally I think adding AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE and use
>>> private_data to hold the codec context is a cleaner/simpler approach.
>>>
>>> - the codec requests private_data with a size (sizeof(type))
>>> - the code explicitly informs the API whether all memory will be released
>>> on close or it will preserve it.
>> - All APIs in ffmpeg with this sort of private data field use them in the
>> same way - they are allocated at create/alloc time (with the given size, for
>> AVOptions), and freed at close/destroy time.
>> - Using the well-tested reference-counted buffer implementation is IMO
>> strongly preferable to making ad-hoc synchronisation with atomics and
>> semaphores.
>> - All other decoders use the reference-counted buffer approach (e.g. look at
>> rkmpp for a direct implementation, the hwaccels all do it via hwcontext).
>
> ok so I guess there is no point to discuss further what I tried to put
> forward (I could provide my implementation to compare against this RFC - it
> is just a handful of lines of code - but I guess there is no point given that
> everyone is comfortable with the current way of doing things.).
>
> so let's make this work then and fix the SIGSEGV present in master asap (btw
> this RFC also seg-fault when the above is applied)
Where does that version segfault? (It doesn't for me.)
> diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
> index 831fd81..1dd8cf0 100644
> --- a/libavcodec/v4l2_m2m_dec.c
> +++ b/libavcodec/v4l2_m2m_dec.c
> @@ -176,8 +176,8 @@ static av_cold int v4l2_decode_init(AVCodecContext *avctx)
> * by the v4l2 driver; this event will trigger a full pipeline reconfig
> and
> * the proper values will be retrieved from the kernel driver.
> */
> - output->height = capture->height = avctx->coded_height;
> - output->width = capture->width = avctx->coded_width;
> + output->height = capture->height = 0; //avctx->coded_height;
> + output->width = capture->width = 0; //avctx->coded_width;
>
> output->av_codec_id = avctx->codec_id;
> output->av_pix_fmt = AV_PIX_FMT_NONE;
Sure, that coded_width/height information is meaningless anyway.
> also looking at your code I think we also need:
>
> [jramirez@igloo libavcodec (patches/m6 *$)]$ git diff v4l2_buffers.c
> diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> index 9831bdb..8dec56d 100644
> --- a/libavcodec/v4l2_buffers.c
> +++ b/libavcodec/v4l2_buffers.c
> @@ -207,15 +207,19 @@ static void v4l2_free_buffer(void *opaque, uint8_t
> *unused)
> V4L2Buffer* avbuf = opaque;
> V4L2m2mContext *s = buf_to_m2mctx(avbuf);
>
> - atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel);
> - if (s->reinit) {
> - if (!atomic_load(&s->refcount))
> - sem_post(&s->refsync);
> - } else if (avbuf->context->streamon) {
> - ff_v4l2_buffer_enqueue(avbuf);
> - }
> + av_log(logger(avbuf), AV_LOG_DEBUG, "free capture %d\n",
> avbuf->buf.index);
>
> if (atomic_fetch_sub(&avbuf->context_refcount, 1) == 1) {
> + atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel);
> +
> + if (s->reinit) {
> + if (!atomic_load(&s->refcount))
> + sem_post(&s->refsync);
> + } else if (avbuf->context->streamon) {
> + av_log(logger(avbuf), AV_LOG_DEBUG, "queue capture %d\n",
> avbuf->buf.index);
> + ff_v4l2_buffer_enqueue(avbuf);
> + }
> +
> av_buffer_unref(&avbuf->context_ref);
> }
> }
I don't think moving it inside the only-run-once-for-each-buffer check like
this works, because the refcount here is incremented once-per-plane rather than
once-per-buffer.
The context_refcount check there is really just a hack around the
once-per-plane behaviour - I think it would probably be better to fix that
first (including the redundant submission), because then this would be easier
to reason about.
>
> I tested the encoder and it seems to work properly (havent checked with
> valgrind but all frames are properly encoded)
>
> how do you want to proceed?
> will you create a branch somewhere and we work on this or should I take it
> and evolve it or will you do it all? thanks!
Feel free to take over the patch. (I believe you have a much better test setup
than I do, and that's probably the most important thing here given the subtle
differences between implementations.)
Thanks,
- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
