| Message ID | b2e4f9e5-5169-83f4-590d-fa7fb7cb99ae@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 10/19/2017 07:55 PM, 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) > > 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; > > > 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 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! > > > fyi testing the h264 m2m encoder ==10241== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==10241== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info ==10241== Command: /home/linaro/Src/git.zoltan.ffmpeg/ffmpeg_g -report -threads 1 -v 55 -f rawvideo -pix_fmt nv12 -s:v 1280:720 -r 25 -i /home/linaro/Videos/raw/freeway.yuv -c:v h264_v4l2m2m out/out.h264.mp4 ==10241== ... ... Input file #0 (/home/linaro/Videos/raw/freeway.yuv): Input stream #0:0 (video): 232 packets read (320716800 bytes); 232 frames decoded; Total: 232 packets (320716800 bytes) demuxed Output file #0 (out/out.h264.mp4): Output stream #0:0 (video): 232 frames encoded; 232 packets muxed (563494 bytes); Total: 232 packets (563494 bytes) muxed 232 frames successfully decoded, 0 decoding errors [AVIOContext @ 0x5aa9270] Statistics: 2 seeks, 6 writeouts [AVIOContext @ 0x59103b0] Statistics: 320716800 bytes read, 0 seeks ==10241== at 0x129F8B8: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303) ==10241== by 0x12A0593: av_log_default_callback (log.c:355) ==10241== by 0x21CEC3: log_callback_report (cmdutils.c:110) ==10241== by 0x12A076F: av_vlog (log.c:383) ==10241== by 0x12A06F7: av_log (log.c:375) ==10241== by 0x224447: term_exit (ffmpeg.c:317) ==10241== by 0x224FCB: ffmpeg_cleanup (ffmpeg.c:618) ==10241== by 0x21CFBB: exit_program (cmdutils.c:138) ==10241== by 0x23412B: main (ffmpeg.c:4832) ==10241== ==10241== HEAP SUMMARY: ==10241== in use at exit: 600 bytes in 2 blocks ==10241== total heap usage: 4,323 allocs, 4,321 frees, 325,278,370 bytes allocated ==10241== ==10241== LEAK SUMMARY: ==10241== definitely lost: 0 bytes in 0 blocks ==10241== indirectly lost: 0 bytes in 0 blocks ==10241== possibly lost: 0 bytes in 0 blocks ==10241== still reachable: 600 bytes in 2 blocks ==10241== suppressed: 0 bytes in 0 blocks ==10241== Reachable blocks (those to which a pointer was found) are not shown. ==10241== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==10241== ==10241== For counts of detected and suppressed errors, rerun with: -v ==10241== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
On 10/19/2017 07:55 PM, Jorge Ramirez wrote: > > 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) dont worry too much about fixing this anyway - this implementation is incorrect and needs a couple of modifications (full reinit currently closes the driver when it should just send the stop command - that was to work around an issue in the venus driver stop ). a fix to venus (db410/db820c hw) has been sent to the kernel mailing list so will validate and fix. > > 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;
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
On 10/23/2017 01:47 AM, Mark Thompson wrote: >> 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.) the patch will segfault if the changes below are applied to the baseline. those changes simulate the case where the client doesnt provide the height/width; when that happens, since ffmpeg registers for the v4l2 kernel event which provides the resolution, ffmpeg will have to dequeue the old buffers and queue new ones capable of storing the frame to the driver (in the code that is handled by reinit/full_reinit. note that full_reinit has not been implemented properly (I had to close the driver to work around a venus issue ...it is an easy fix once I have a driver that works) >> 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;
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; 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); + } +