Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: fix hw_device_ctx leak
On Fri, Dec 3, 2021, at 09:52, Anton Khirnov wrote: > Quoting Thomas Guillem (2021-12-02 20:44:33) >> >> On Thu, Dec 2, 2021, at 15:05, Andreas Rheinhardt wrote: >> > 1. hw_device_ctx is in the public AVCodecContext, not in the private >> > AVCodecInternal. >> > 2. ctx->internal is intended to be allocated for all the internal >> > AVCodecContexts; the check is only there to ensure that we don't crash >> > if an allocation error happens. >> > 3. libavcodec only ever modifies avctx->hw_device_ctx at one place: in >> > avcodec_close() where is unreferences the reference (if any). So if it >> > was set in avcodec_open2() and NULL in avcodec_close() (i.e. immediately >> > before avcodec_close()), it has been modified somehow and you need to >> > find out who did it. >> > 4. Some parts of the code use hw_device_ctx to derive references from >> > it. Maybe one of these references did not get freed? >> > (5. What do Valgrind/ASAN say? When was the reference that leaked created?) >> >> OK, all your point are valid, I missed a precious comment from the avcodec.h >> API: >> "For both encoders and decoders this field should be set before >> avcodec_open2() is called and must not be written to thereafter." >> >> Hence, the incomprehension. >> >> So, to sum up: the reference that leaks is created from VLC, from the >> get_format() callback. >> Looking at pthread_frame.c, it seems that get_format() is called with a >> AVCodecContext owned by a PerThreadContext, so not the main AVCodecContext. >> That is why avcodec_close() is not closing it. >> >> I just discovered that it's not officially supported then, but I really >> don't see any other way to plug VAAPI between VLC and ffmpeg. >> >> I explain: VLC can't know the hw_device that will be used when it calls >> avcodec_open2(). VLC has a similar struct than hw_device, called dec_device. >> For VAAPI, this struct hold the va_dpy that is created when the VLC avcodec >> module first create the video output, from the get_format() callback. >> >> I see 2 possibilities: >> 1/ Patch the documentation and allow to set a hw_device_ctx from >> get_format(), review all use case of hw_device_ctx to check if there is no >> inconsistency with this new change. >> 2/ Use any other way to pass the va_dpy from the get_format()? I didn't see >> any, but maybe I missed something obvious. > > 2. is the right answer - you're supposed to set hw_frames_ctx, possibly > using the avcodec_get_hw_frames_parameters() helper function. Thanks! That is exactly what I needed. > > -- > Anton Khirnov > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: fix hw_device_ctx leak
Quoting Thomas Guillem (2021-12-02 20:44:33) > > On Thu, Dec 2, 2021, at 15:05, Andreas Rheinhardt wrote: > > 1. hw_device_ctx is in the public AVCodecContext, not in the private > > AVCodecInternal. > > 2. ctx->internal is intended to be allocated for all the internal > > AVCodecContexts; the check is only there to ensure that we don't crash > > if an allocation error happens. > > 3. libavcodec only ever modifies avctx->hw_device_ctx at one place: in > > avcodec_close() where is unreferences the reference (if any). So if it > > was set in avcodec_open2() and NULL in avcodec_close() (i.e. immediately > > before avcodec_close()), it has been modified somehow and you need to > > find out who did it. > > 4. Some parts of the code use hw_device_ctx to derive references from > > it. Maybe one of these references did not get freed? > > (5. What do Valgrind/ASAN say? When was the reference that leaked created?) > > OK, all your point are valid, I missed a precious comment from the avcodec.h > API: > "For both encoders and decoders this field should be set before > avcodec_open2() is called and must not be written to thereafter." > > Hence, the incomprehension. > > So, to sum up: the reference that leaks is created from VLC, from the > get_format() callback. > Looking at pthread_frame.c, it seems that get_format() is called with a > AVCodecContext owned by a PerThreadContext, so not the main AVCodecContext. > That is why avcodec_close() is not closing it. > > I just discovered that it's not officially supported then, but I really don't > see any other way to plug VAAPI between VLC and ffmpeg. > > I explain: VLC can't know the hw_device that will be used when it calls > avcodec_open2(). VLC has a similar struct than hw_device, called dec_device. > For VAAPI, this struct hold the va_dpy that is created when the VLC avcodec > module first create the video output, from the get_format() callback. > > I see 2 possibilities: > 1/ Patch the documentation and allow to set a hw_device_ctx from > get_format(), review all use case of hw_device_ctx to check if there is no > inconsistency with this new change. > 2/ Use any other way to pass the va_dpy from the get_format()? I didn't see > any, but maybe I missed something obvious. 2. is the right answer - you're supposed to set hw_frames_ctx, possibly using the avcodec_get_hw_frames_parameters() helper function. -- Anton Khirnov ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: fix hw_device_ctx leak
On Thu, Dec 2, 2021, at 15:05, Andreas Rheinhardt wrote: > 1. hw_device_ctx is in the public AVCodecContext, not in the private > AVCodecInternal. > 2. ctx->internal is intended to be allocated for all the internal > AVCodecContexts; the check is only there to ensure that we don't crash > if an allocation error happens. > 3. libavcodec only ever modifies avctx->hw_device_ctx at one place: in > avcodec_close() where is unreferences the reference (if any). So if it > was set in avcodec_open2() and NULL in avcodec_close() (i.e. immediately > before avcodec_close()), it has been modified somehow and you need to > find out who did it. > 4. Some parts of the code use hw_device_ctx to derive references from > it. Maybe one of these references did not get freed? > (5. What do Valgrind/ASAN say? When was the reference that leaked created?) OK, all your point are valid, I missed a precious comment from the avcodec.h API: "For both encoders and decoders this field should be set before avcodec_open2() is called and must not be written to thereafter." Hence, the incomprehension. So, to sum up: the reference that leaks is created from VLC, from the get_format() callback. Looking at pthread_frame.c, it seems that get_format() is called with a AVCodecContext owned by a PerThreadContext, so not the main AVCodecContext. That is why avcodec_close() is not closing it. I just discovered that it's not officially supported then, but I really don't see any other way to plug VAAPI between VLC and ffmpeg. I explain: VLC can't know the hw_device that will be used when it calls avcodec_open2(). VLC has a similar struct than hw_device, called dec_device. For VAAPI, this struct hold the va_dpy that is created when the VLC avcodec module first create the video output, from the get_format() callback. I see 2 possibilities: 1/ Patch the documentation and allow to set a hw_device_ctx from get_format(), review all use case of hw_device_ctx to check if there is no inconsistency with this new change. 2/ Use any other way to pass the va_dpy from the get_format()? I didn't see any, but maybe I missed something obvious. Best, Thomas > > - Andreas > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: fix hw_device_ctx leak
Thomas Guillem: > > > On Thu, Dec 2, 2021, at 14:38, Andreas Rheinhardt wrote: >> Thomas Guillem: >>> Reproduced when using the VAAPI va module on VLC 4.0. No leaks when >>> setting thread count to 1. >>> --- >>> libavcodec/pthread_frame.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >>> index 73b1b7d7d9..4c578aa44a 100644 >>> --- a/libavcodec/pthread_frame.c >>> +++ b/libavcodec/pthread_frame.c >>> @@ -747,6 +747,7 @@ void ff_frame_thread_free(AVCodecContext *avctx, int >>> thread_count) >>> av_buffer_unref(&ctx->internal->pool); >>> av_freep(&ctx->internal); >>> av_buffer_unref(&ctx->hw_frames_ctx); >>> +av_buffer_unref(&ctx->hw_device_ctx); >>> } >>> >>> av_frame_free(&p->frame); >>> >> >> The AVCodecContext that is freed here is not a full AVCodecContext: It >> never received a reference to hw_device_ctx of its own. Unreferencing >> this here will therefore mess up the refcount and lead to use-after-frees. >> (What is the reference count of hw_device_ctx at this point? Libavcodec >> should only hold one reference at that point, namely the one in the main >> (user-facing) AVCodecContext; this reference will be unreferenced when >> avcodec_close()/avcodec_free_context() is called for the main context.) > > Hello Andreas, thanks for the review. > > The problem is that hw_device_ctx is NULL when avcodec_close() is called in > that particular usecase. My first guess is that the get_format callback can > be called from any avctx (and not necessarily the main one that will be > closed by avcodec_close(). > > I'm new to this code, but I thought that only one FrameThreadContext could > meet the if (ctx->internal) condition, so there should be only one reference > to the hw_device_ctx. > 1. hw_device_ctx is in the public AVCodecContext, not in the private AVCodecInternal. 2. ctx->internal is intended to be allocated for all the internal AVCodecContexts; the check is only there to ensure that we don't crash if an allocation error happens. 3. libavcodec only ever modifies avctx->hw_device_ctx at one place: in avcodec_close() where is unreferences the reference (if any). So if it was set in avcodec_open2() and NULL in avcodec_close() (i.e. immediately before avcodec_close()), it has been modified somehow and you need to find out who did it. 4. Some parts of the code use hw_device_ctx to derive references from it. Maybe one of these references did not get freed? (5. What do Valgrind/ASAN say? When was the reference that leaked created?) - Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: fix hw_device_ctx leak
On Thu, Dec 2, 2021, at 14:38, Andreas Rheinhardt wrote: > Thomas Guillem: >> Reproduced when using the VAAPI va module on VLC 4.0. No leaks when >> setting thread count to 1. >> --- >> libavcodec/pthread_frame.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >> index 73b1b7d7d9..4c578aa44a 100644 >> --- a/libavcodec/pthread_frame.c >> +++ b/libavcodec/pthread_frame.c >> @@ -747,6 +747,7 @@ void ff_frame_thread_free(AVCodecContext *avctx, int >> thread_count) >> av_buffer_unref(&ctx->internal->pool); >> av_freep(&ctx->internal); >> av_buffer_unref(&ctx->hw_frames_ctx); >> +av_buffer_unref(&ctx->hw_device_ctx); >> } >> >> av_frame_free(&p->frame); >> > > The AVCodecContext that is freed here is not a full AVCodecContext: It > never received a reference to hw_device_ctx of its own. Unreferencing > this here will therefore mess up the refcount and lead to use-after-frees. > (What is the reference count of hw_device_ctx at this point? Libavcodec > should only hold one reference at that point, namely the one in the main > (user-facing) AVCodecContext; this reference will be unreferenced when > avcodec_close()/avcodec_free_context() is called for the main context.) Hello Andreas, thanks for the review. The problem is that hw_device_ctx is NULL when avcodec_close() is called in that particular usecase. My first guess is that the get_format callback can be called from any avctx (and not necessarily the main one that will be closed by avcodec_close(). I'm new to this code, but I thought that only one FrameThreadContext could meet the if (ctx->internal) condition, so there should be only one reference to the hw_device_ctx. Best, Thomas > > - Andreas > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: fix hw_device_ctx leak
Thomas Guillem: > Reproduced when using the VAAPI va module on VLC 4.0. No leaks when > setting thread count to 1. > --- > libavcodec/pthread_frame.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index 73b1b7d7d9..4c578aa44a 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -747,6 +747,7 @@ void ff_frame_thread_free(AVCodecContext *avctx, int > thread_count) > av_buffer_unref(&ctx->internal->pool); > av_freep(&ctx->internal); > av_buffer_unref(&ctx->hw_frames_ctx); > +av_buffer_unref(&ctx->hw_device_ctx); > } > > av_frame_free(&p->frame); > The AVCodecContext that is freed here is not a full AVCodecContext: It never received a reference to hw_device_ctx of its own. Unreferencing this here will therefore mess up the refcount and lead to use-after-frees. (What is the reference count of hw_device_ctx at this point? Libavcodec should only hold one reference at that point, namely the one in the main (user-facing) AVCodecContext; this reference will be unreferenced when avcodec_close()/avcodec_free_context() is called for the main context.) - Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
