Re: [FFmpeg-devel] [PATCH] lavc/videotoolbox: validate vt context in the decoder callback
Ping on this, Can I contribute somehow either to fix calling `av_videotoolbox_default_init` or at least update the docs? Best Regards, Alessandro On 11 Dec 2022, 11:57 +0200, Alessandro Di Nepi , wrote: > On 9 Dec 2022, 6:45 +0200, FFmpeg development discussions and patches > , wrote: > > Did you call av_videotoolbox_default_init() or > > av_videotoolbox_default_init2()? > Actually yes, I call `av_videotoolbox_default_init(context);` > > I think that’s why avctx->internal->hwaccel_priv_data is NULL. That code > > path is broken. > > Does remove the call of > > av_videotoolbox_default_init()/av_videotoolbox_default_init2() works for > > you? > Definitively yes, if I remove the call to `av_videotoolbox_default_init` the > context is fine. > > Thanks for sorting this out. I'd like to contribute and have this fixed or > documented for other people. > What should be the desired behavior? Calling `av_videotoolbox_default_init` > and having the context not NULL? > Or shall we remove the init at all? > > Best Regards ___ 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] lavc/videotoolbox: validate vt context in the decoder callback
On 9 Dec 2022, 6:45 +0200, FFmpeg development discussions and patches , wrote: > Did you call av_videotoolbox_default_init() or > av_videotoolbox_default_init2()? Actually yes, I call `av_videotoolbox_default_init(context);` > I think that’s why avctx->internal->hwaccel_priv_data is NULL. That code path > is broken. > Does remove the call of > av_videotoolbox_default_init()/av_videotoolbox_default_init2() works for you? Definitively yes, if I remove the call to `av_videotoolbox_default_init` the context is fine. Thanks for sorting this out. I'd like to contribute and have this fixed or documented for other people. What should be the desired behavior? Calling `av_videotoolbox_default_init` and having the context not NULL? Or shall we remove the init at all? Best Regards ___ 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] lavc/videotoolbox: validate vt context in the decoder callback
Got you; giving some context here, and you can find all the details in the ticket #10079 (http://trac.ffmpeg.org/ticket/10079). The issue has been introduced with the commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce. This patch basically changed: • In the function `videotoolbox_start(AVCodecContext *avctx)`, ``` - decoder_cb.decompressionOutputRefCon = avctx; + decoder_cb.decompressionOutputRefCon = avctx->internal->hwaccel_priv_data; ``` • The context is retrieved in the function, `videotoolbox_decoder_callback(...)` ``` - AVCodecContext *avctx = opaque; - VTContext *vtctx = avctx->internal->hwaccel_priv_data; + VTContext *vtctx = opaque; ``` Having said that, I see that when the `videotoolbox_start` is called, • `avctx` is not NULL, • `avctx->internal->hwaccel_priv_data` is NULL The first time the `videotoolbox_decoder_callback` is called, `avctx->internal->hwaccel_priv_data` now has a value, so before d7f4ad88a `vtctx` has a value. After the change, since `avctx->internal->hwaccel_priv_data` is captured in `video toolbox_start`, is NULL and `vtctx` is also NULL. Again, this happens just the first time the callback is called; from the second time, vtctx has a proper value, and everything proceeds as expected. I'm willing to change the patch if you think there is a better way, but something needs to be done because the library simply crashes in the current state. From what I see from the original change, reverting is not an option. Looking forward to hear feedback on this. Best Regards Alessandro On 6 Dec 2022, 7:20 +0200, "zhilizhao(赵志立)" , wrote: > > > On Dec 5, 2022, at 21:36, Rick Kern wrote: > > > > On Sun, Dec 4, 2022 at 12:51 PM Alessandro Di Nepi < > > alessandro.din...@gmail.com> wrote: > > > > > On 4 Dec 2022, 17:01 +0200, FFmpeg development discussions and patches < > > > ffmpeg-devel@ffmpeg.org>, wrote: > > > > When this happens, does it continue happening, or is it transient? My > > > main > > > > concern is log spamming. > > > Good question: this is just a transient state, so that it won't continue > > > happening. > > > To give you some context: when the decoding start, the value of `vtctx` is > > > captured "too" early so that the first time the callback is called, it's > > > still NULL. > > > The next time it will have a proper value. > > > > > If the code isn't setting a variable in time, that issue should be fixed. > > Otherwise the decoder will drop frames. > > Yes, null pointer check doesn’t looks like a resolution to a race > condition. I’m not sure how the race condition happened in the first > place. > ___ 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] lavc/videotoolbox: validate vt context in the decoder callback
On 4 Dec 2022, 17:01 +0200, FFmpeg development discussions and patches , wrote: > When this happens, does it continue happening, or is it transient? My main > concern is log spamming. Good question: this is just a transient state, so that it won't continue happening. To give you some context: when the decoding start, the value of `vtctx` is captured "too" early so that the first time the callback is called, it's still NULL. The next time it will have a proper value. ___ 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] lavc/videotoolbox: validate vt context in the decoder callback
A gentle reminder for this patch: it's a simple fix that prevents crashing on iOS. Thanks ___ 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] lavc/videotoolbox: validate vt context in the decoder callback
Just to add that this fix, once approved, should be cherry-picked to all the
release branches where d7f4ad88a0df3c1339e142957bf2c40cd056b8ce has been
cherry-picked.
Basically, 4.4, 5.0, and 5.1.
Thanks
On 27 Nov 2022, 19:34 +0200, Alessandro Di Nepi ,
wrote:
> The commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce introduced a race
> condition where the passed opaque pointer reference might be NULL,
> when the decoding process starts.
> This patch checks that vtctx has a value before accessing it.
>
> This patch fixes #10079.
>
> Signed-off-by: Alessandro Di Nepi
> ---
> libavcodec/videotoolbox.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> index 1b1be8ddb4..615e2b087a 100644
> --- a/libavcodec/videotoolbox.c
> +++ b/libavcodec/videotoolbox.c
> @@ -692,6 +692,11 @@ static void videotoolbox_decoder_callback(void *opaque,
> {
> VTContext *vtctx = opaque;
>
> + if (!vtctx) {
> + av_log(NULL, AV_LOG_WARNING, "vt decoder cb: vt context is null");
> + return;
> + }
> +
> if (vtctx->frame) {
> CVPixelBufferRelease(vtctx->frame);
> vtctx->frame = NULL;
> --
> 2.37.1 (Apple Git-137.1)
>
___
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] [PATCH] lavc/videotoolbox: validate vt context in the decoder callback
The commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce introduced a race
condition where the passed opaque pointer reference might be NULL,
when the decoding process starts.
This patch checks that vtctx has a value before accessing it.
This patch fixes #10079.
Signed-off-by: Alessandro Di Nepi
---
libavcodec/videotoolbox.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
index 1b1be8ddb4..615e2b087a 100644
--- a/libavcodec/videotoolbox.c
+++ b/libavcodec/videotoolbox.c
@@ -692,6 +692,11 @@ static void videotoolbox_decoder_callback(void *opaque,
{
VTContext *vtctx = opaque;
+if (!vtctx) {
+av_log(NULL, AV_LOG_WARNING, "vt decoder cb: vt context is null");
+return;
+}
+
if (vtctx->frame) {
CVPixelBufferRelease(vtctx->frame);
vtctx->frame = NULL;
--
2.37.1 (Apple Git-137.1)
___
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] [PATCH] lavc/videotoolbox: validate vt context in the decoder callback
The commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce introduced a race
condition where the passed opaque pointer reference might be NULL,
when the decoding process starts.
This patch checks that vtctx has a value before accessing it.
This patch fixes #10079.
Signed-off-by: Alessandro Di Nepi
---
libavcodec/videotoolbox.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
index 1b1be8ddb4..615e2b087a 100644
--- a/libavcodec/videotoolbox.c
+++ b/libavcodec/videotoolbox.c
@@ -692,6 +692,11 @@ static void videotoolbox_decoder_callback(void *opaque,
{
VTContext *vtctx = opaque;
+ if (!vtctx) {
+ av_log(NULL, AV_LOG_WARNING, "vt decoder cb: vt context is null");
+ return;
+ }
+
if (vtctx->frame) {
CVPixelBufferRelease(vtctx->frame);
vtctx->frame = NULL;
--
2.37.1 (Apple Git-137.1)
___
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".
