Message ID | 716998d4-3307-de66-7b29-00ed5e43ce9f@linaro.org |
---|---|
State | New |
Headers | show |
On 18/10/17 08:46, Jorge Ramirez wrote: > On 10/18/2017 12:34 AM, Mark Thompson wrote: >>> 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. >> > > right. as a matter of fact, this synchronization can not be done using private data (the opaque structure passed to the callback must contain all the required information (and updated!) at any time). On top of this being hard to implement would complicate the current code quite a bit making it also very hard to maintain. So agreed. > > So back to a previous proposal, would the flag below be acceptable? [1] > > Seems sensible to me to give responsibility of the private data deallocation to the codec itself independently of the close function since the buffers allocated by the codec controlled by a separate thread will need access to that data. > > Put differently since the close function can not be made responsible for deallocating mmap/queued memory in V4L2 (or even closing the kernel driver) when the user keeps buffers, it should not be allowed to delete the codec private data since in my view it forces the design to take a lot of unnecessary actions for no real reason. > > With the proposal below, the code remains cleaner and free of race conditions. > > [1] .. > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 52cc5b0..0a3ca0c 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -982,6 +982,11 @@ typedef struct RcOverride{ > * Do not reset ASS ReadOrder field on flush (subtitles decoding) > */ > #define AV_CODEC_FLAG2_RO_FLUSH_NOOP (1 << 30) > +/** > + * Closing the codec doesnt release the context priv_data (it becomes the codec > + * responsibility) > + */ > +#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE (1 << 31) > > /* Unsupported options : > * Syntax Arithmetic coding (SAC) > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 9551f31..b6c5adf 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx) > if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) > av_opt_free(avctx->priv_data); > av_opt_free(avctx); > - av_freep(&avctx->priv_data); > + if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE)) > + av_freep(&avctx->priv_data); > + > > I'm not seeing the motivation for this at all. Anything which possibly needs to persist beyond the codec close will be refcounted (with references from the codec instance itself and from the outstanding output frames, as you have), so it should be stored in a refcounted buffer. That's how all other components with this problem work, so why does v4l2 want special API to behave differently? - Mark
On 10/18/2017 03:28 PM, Mark Thompson wrote: > On 18/10/17 08:46, Jorge Ramirez wrote: >> On 10/18/2017 12:34 AM, Mark Thompson wrote: >>>> 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. >>> >> right. as a matter of fact, this synchronization can not be done using private data (the opaque structure passed to the callback must contain all the required information (and updated!) at any time). On top of this being hard to implement would complicate the current code quite a bit making it also very hard to maintain. So agreed. >> >> So back to a previous proposal, would the flag below be acceptable? [1] >> >> Seems sensible to me to give responsibility of the private data deallocation to the codec itself independently of the close function since the buffers allocated by the codec controlled by a separate thread will need access to that data. >> >> Put differently since the close function can not be made responsible for deallocating mmap/queued memory in V4L2 (or even closing the kernel driver) when the user keeps buffers, it should not be allowed to delete the codec private data since in my view it forces the design to take a lot of unnecessary actions for no real reason. >> >> With the proposal below, the code remains cleaner and free of race conditions. >> >> [1] .. >> >> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >> index 52cc5b0..0a3ca0c 100644 >> --- a/libavcodec/avcodec.h >> +++ b/libavcodec/avcodec.h >> @@ -982,6 +982,11 @@ typedef struct RcOverride{ >> * Do not reset ASS ReadOrder field on flush (subtitles decoding) >> */ >> #define AV_CODEC_FLAG2_RO_FLUSH_NOOP (1 << 30) >> +/** >> + * Closing the codec doesnt release the context priv_data (it becomes the codec >> + * responsibility) >> + */ >> +#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE (1 << 31) >> >> /* Unsupported options : >> * Syntax Arithmetic coding (SAC) >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >> index 9551f31..b6c5adf 100644 >> --- a/libavcodec/utils.c >> +++ b/libavcodec/utils.c >> @@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx) >> if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) >> av_opt_free(avctx->priv_data); >> av_opt_free(avctx); >> - av_freep(&avctx->priv_data); >> + if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE)) >> + av_freep(&avctx->priv_data); >> + >> >> > I'm not seeing the motivation for this at all. right. Yes, the motivation is implicit in the way v4l2 support was implemented (capture and output context are allocated on priv_data (they are not pointers) to enable navigating the data structures using container_of all over the code). this makes the code not only fast and less prone to leaks but also easier to read and maintain (it is a pattern used in many drivers) The v4l2 implementation only allocates two chunks of heap to accommodate all of its buffers. So leaks are extremely unlikely. > Anything which possibly needs to persist beyond the codec close will be refcounted (with references from the codec instance itself and from the outstanding output frames, as you have), so it should be stored in a refcounted buffer. Not if the codec puts all its information on private_data as is done in v4l2. > That's how all other components with this problem work, so why does v4l2 want special API to behave differently? Given the use case we are implementing against, I think it is sensible to give the codec the 'option' to handle its private_data (ie, the patch above) But I would also like to understand your concern before replacing the structures with pointers and rewriting most of the code. I think that the current codec API is not complete and the fact that reference counting of buffers after codec close must be implemented (if this was actually the reason) is probably symptom. So I am not really proposing an especial API for v4l2 but an enhancement of the current one to make it easier to use. But let me explain further The API itself is quite common: - provides an interface to allocate private data with a size. - provides generic interface (close) to deallocate private_data _along_ with everything else. This is standard to many frameworks but those frameworks typically expect the close function to release all allocated memory done during init However the ffmpeg use case requires that closing the codec doesnt release all memory (ie buffers); that can't be done unless i) private_data persists under codec control (patch above so each codec can select whether to manage the priv_date lifetime or not) OR ii) we avoid placing critical stuff on priv_data and implement reference counting to anything the codec needs for its buffers houskeeping. I am repeating myself here but there are two ways to implement the use case above: 1. an opaque on the buffer allocation callback function that carries all the information (codec state, file descriptors, whatever) that the buffer needs to do the house keeping. 2. an opaque on the buffer allocation callback function that points to the codec private_data (therefore gaining access to all the codec information that it needs). why is it a problem to provide both and let each codec chose depending on its implementation? In the case of v4l2: 1. the code is navigated by using container_of (we could use pointers instead but I think that adds noise to the definition of the structures) 2. each capture buffer released by the client needs to know a) if the codec was closed b) if the codec is reinitializing (dequeing all buffers to enqueue buffers with a new size again) c) if the buffer needs to be queued again a) if the codec was closed it needs to know if it is the last buffer in which case it unmaps all the buffers from the process and asks the driver to release them. b) it needs to know if it is the last one to allow the re-initialization to continue (post a semaphore) c) if we are still streaming, it needs access to the driver to queue again. All this can be maintained easily in private_data without adding any complexity really. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Wed, Oct 18, 2017 at 09:46:40AM +0200, Jorge Ramirez wrote: > On 10/18/2017 12:34 AM, Mark Thompson wrote: > >> 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. > > > > right. as a matter of fact, this synchronization can not be done > using private data (the opaque structure passed to the callback must > contain all the required information (and updated!) at any time). On > top of this being hard to implement would complicate the current > code quite a bit making it also very hard to maintain. So agreed. > > So back to a previous proposal, would the flag below be acceptable? [1] > > Seems sensible to me to give responsibility of the private data > deallocation to the codec itself independently of the close function > since the buffers allocated by the codec controlled by a separate > thread will need access to that data. > > Put differently since the close function can not be made responsible > for deallocating mmap/queued memory in V4L2 (or even closing the > kernel driver) when the user keeps buffers, it should not be allowed > to delete the codec private data since in my view it forces the > design to take a lot of unnecessary actions for no real reason. > > With the proposal below, the code remains cleaner and free of race > conditions. > > [1] .. > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 52cc5b0..0a3ca0c 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -982,6 +982,11 @@ typedef struct RcOverride{ > * Do not reset ASS ReadOrder field on flush (subtitles decoding) > */ > #define AV_CODEC_FLAG2_RO_FLUSH_NOOP (1 << 30) > +/** > + * Closing the codec doesnt release the context priv_data (it > becomes the codec > + * responsibility) > + */ > +#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE (1 << 31) > > /* Unsupported options : > * Syntax Arithmetic coding (SAC) > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 9551f31..b6c5adf 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx) > if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) > av_opt_free(avctx->priv_data); > av_opt_free(avctx); > - av_freep(&avctx->priv_data); > + if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE)) > + av_freep(&avctx->priv_data); This may be solving a similar issue as our udp.c cleanup with its thread. Not sure, i didnt follow this discussion closely. just wanted to mention as one migt benefit from the other if its the same issue. Another thing that came to my mind, if the context (and other resources) are deallocated with a delay beyond closing. Is this still correct if the process terminates before or in the middle. Or is that preented somehow ? There would be at least spurious warnings from memory debuggers if deallocation isnt run at all [...]
On 10/18/2017 09:40 PM, Michael Niedermayer wrote: > On Wed, Oct 18, 2017 at 09:46:40AM +0200, Jorge Ramirez wrote: >> On 10/18/2017 12:34 AM, Mark Thompson wrote: >>>> 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. >>> >> right. as a matter of fact, this synchronization can not be done >> using private data (the opaque structure passed to the callback must >> contain all the required information (and updated!) at any time). On >> top of this being hard to implement would complicate the current >> code quite a bit making it also very hard to maintain. So agreed. >> >> So back to a previous proposal, would the flag below be acceptable? [1] >> >> Seems sensible to me to give responsibility of the private data >> deallocation to the codec itself independently of the close function >> since the buffers allocated by the codec controlled by a separate >> thread will need access to that data. >> >> Put differently since the close function can not be made responsible >> for deallocating mmap/queued memory in V4L2 (or even closing the >> kernel driver) when the user keeps buffers, it should not be allowed >> to delete the codec private data since in my view it forces the >> design to take a lot of unnecessary actions for no real reason. >> >> With the proposal below, the code remains cleaner and free of race >> conditions. >> >> [1] .. >> >> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >> index 52cc5b0..0a3ca0c 100644 >> --- a/libavcodec/avcodec.h >> +++ b/libavcodec/avcodec.h >> @@ -982,6 +982,11 @@ typedef struct RcOverride{ >> * Do not reset ASS ReadOrder field on flush (subtitles decoding) >> */ >> #define AV_CODEC_FLAG2_RO_FLUSH_NOOP (1 << 30) >> +/** >> + * Closing the codec doesnt release the context priv_data (it >> becomes the codec >> + * responsibility) >> + */ >> +#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE (1 << 31) >> >> /* Unsupported options : >> * Syntax Arithmetic coding (SAC) >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >> index 9551f31..b6c5adf 100644 >> --- a/libavcodec/utils.c >> +++ b/libavcodec/utils.c >> @@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx) >> if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) >> av_opt_free(avctx->priv_data); >> av_opt_free(avctx); >> - av_freep(&avctx->priv_data); >> + if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE)) >> + av_freep(&avctx->priv_data); > > This may be solving a similar issue as our udp.c cleanup with its > thread. Not sure, i didnt follow this discussion closely. > just wanted to mention as one migt benefit from the other if its the > same issue. > > Another thing that came to my mind, if the context (and other resources) > are deallocated with a delay beyond closing. Is this still correct if > the process terminates before or in the middle. > Or is that preented somehow ? sorry not sure I follow, since the buffer references also belong to the process closing the process would end up closing the file descriptor (ie v4l2 driver). the driver would have to do its internal house keeping (release its queued buffers and so on). I dont think this would be an issue. > There would be at least spurious warnings from memory debuggers if > deallocation isnt run at all but the change above is not functionally different to what we do today: just gives the codec more control over its private_data > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, Oct 19, 2017 at 09:01:36AM +0200, Jorge Ramirez wrote: > On 10/18/2017 09:40 PM, Michael Niedermayer wrote: > >On Wed, Oct 18, 2017 at 09:46:40AM +0200, Jorge Ramirez wrote: > >>On 10/18/2017 12:34 AM, Mark Thompson wrote: > >>>> 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. > >>> > >>right. as a matter of fact, this synchronization can not be done > >>using private data (the opaque structure passed to the callback must > >>contain all the required information (and updated!) at any time). On > >>top of this being hard to implement would complicate the current > >>code quite a bit making it also very hard to maintain. So agreed. > >> > >>So back to a previous proposal, would the flag below be acceptable? [1] > >> > >>Seems sensible to me to give responsibility of the private data > >>deallocation to the codec itself independently of the close function > >>since the buffers allocated by the codec controlled by a separate > >>thread will need access to that data. > >> > >>Put differently since the close function can not be made responsible > >>for deallocating mmap/queued memory in V4L2 (or even closing the > >>kernel driver) when the user keeps buffers, it should not be allowed > >>to delete the codec private data since in my view it forces the > >>design to take a lot of unnecessary actions for no real reason. > >> > >>With the proposal below, the code remains cleaner and free of race > >>conditions. > >> > >>[1] .. > >> > >>diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > >>index 52cc5b0..0a3ca0c 100644 > >>--- a/libavcodec/avcodec.h > >>+++ b/libavcodec/avcodec.h > >>@@ -982,6 +982,11 @@ typedef struct RcOverride{ > >> * Do not reset ASS ReadOrder field on flush (subtitles decoding) > >> */ > >> #define AV_CODEC_FLAG2_RO_FLUSH_NOOP (1 << 30) > >>+/** > >>+ * Closing the codec doesnt release the context priv_data (it > >>becomes the codec > >>+ * responsibility) > >>+ */ > >>+#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE (1 << 31) > >> > >> /* Unsupported options : > >> * Syntax Arithmetic coding (SAC) > >>diff --git a/libavcodec/utils.c b/libavcodec/utils.c > >>index 9551f31..b6c5adf 100644 > >>--- a/libavcodec/utils.c > >>+++ b/libavcodec/utils.c > >>@@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx) > >> if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) > >> av_opt_free(avctx->priv_data); > >> av_opt_free(avctx); > >>- av_freep(&avctx->priv_data); > >>+ if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE)) > >>+ av_freep(&avctx->priv_data); > > > >This may be solving a similar issue as our udp.c cleanup with its > >thread. Not sure, i didnt follow this discussion closely. > >just wanted to mention as one migt benefit from the other if its the > >same issue. > > > >Another thing that came to my mind, if the context (and other resources) > >are deallocated with a delay beyond closing. Is this still correct if > >the process terminates before or in the middle. > >Or is that preented somehow ? > > sorry not sure I follow, since the buffer references also belong to > the process closing the process would end up closing the file > descriptor (ie v4l2 driver). > the driver would have to do its internal house keeping (release its > queued buffers and so on). > I dont think this would be an issue. for output devices, terminating the process before all data is output would lead to data loss, this applies to the udp case. Other cases may or may not be affected by this > > >There would be at least spurious warnings from memory debuggers if > >deallocation isnt run at all > > but the change above is not functionally different to what we do > today: just gives the codec more control over its private_data I didnt mean to suggest that this would change it. Just that solutions like this could have this issue [...]
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 52cc5b0..0a3ca0c 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -982,6 +982,11 @@ typedef struct RcOverride{ * Do not reset ASS ReadOrder field on flush (subtitles decoding) */ #define AV_CODEC_FLAG2_RO_FLUSH_NOOP (1 << 30) +/** + * Closing the codec doesnt release the context priv_data (it becomes the codec + * responsibility) + */ +#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE (1 << 31) /* Unsupported options : * Syntax Arithmetic coding (SAC) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 9551f31..b6c5adf 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx) if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) av_opt_free(avctx->priv_data); av_opt_free(avctx); - av_freep(&avctx->priv_data); + if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE)) + av_freep(&avctx->priv_data); + _______________________________________________