diff mbox series

[FFmpeg-devel] avcodec/mediacodecdec: call MediaCodec.stop on close

Message ID DU0PR03MB956744CBEDE6FC5EB5AF8B88ECB82@DU0PR03MB9567.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel] avcodec/mediacodecdec: call MediaCodec.stop on close | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished

Commit Message

sfan5 Aug. 7, 2024, 4:27 p.m. UTC 
Hi all,

attached is a small fix for the MediaCodec code. Tested on Android 14.

Comments

Matthieu Bouron Aug. 7, 2024, 7:11 p.m. UTC | #1 
On Wed, Aug 7, 2024 at 6:28 PM sfan5 <sfan5@live.de> wrote:
>
> Hi all,
>
> attached is a small fix for the MediaCodec code. Tested on Android 14.
>

LGTM, will apply in a few days.
Zhao Zhili Aug. 8, 2024, 3:29 p.m. UTC | #2 
> On Aug 8, 2024, at 00:27, sfan5 <sfan5@live.de> wrote:
> 
> Hi all,
> 
> attached is a small fix for the MediaCodec code. Tested on Android 14.

> This can free up vital resources in case of using multiple
> decoding instances and there are buffer references left over
> and not immediately cleaned up.
> 
> Signed-off-by: sfan5 <sfan5@live.de>
> ---
>  libavcodec/mediacodecdec_common.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/libavcodec/mediacodecdec_common.c b/libavcodec/mediacodecdec_common.c
> index d6f91e6e89..c888dea8cf 100644
> --- a/libavcodec/mediacodecdec_common.c
> +++ b/libavcodec/mediacodecdec_common.c
> @@ -841,6 +841,18 @@ int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
>  
>  int ff_mediacodec_dec_close(AVCodecContext *avctx, MediaCodecDecContext *s)
>  {
> +    if (!s)
> +        return 0;
> +
> +    if (s->codec) {
> +        if (atomic_load(&s->hw_buffer_count) == 0) {
> +            ff_AMediaCodec_stop(s->codec);
> +            av_log(avctx, AV_LOG_DEBUG, "MediaCodec %p stopped\n", s->codec);
> +        } else {
> +            av_log(avctx, AV_LOG_DEBUG, "Not stopping MediaCodec (there are buffers pending)\n");
> +        }
> +    }
> +

Could you elaborate on how this resolved the issue?
If hw_buffer_count is zero, isn’t MediaCodecDecContext will be released immediately?
If hw_buffer_count isn’t zero, the patch make no real change, so how does this patch work?

>      ff_mediacodec_dec_unref(s);
>  
>      return 0;
> -- 
> 2.46.0
> 

> 
> <0001-avcodec-mediacodecdec-call-MediaCodec.stop-on-close.patch>_______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
sfan5 Aug. 8, 2024, 6:14 p.m. UTC | #3 
Am 08.08.24 um 17:29 schrieb Zhao Zhili:
>> On Aug 8, 2024, at 00:27, sfan5<sfan5@live.de>  wrote:
>>
>> Hi all,
>>
>> attached is a small fix for the MediaCodec code. Tested on Android 14.
>>
>> This can free up vital resources in case of using multiple
>> decoding instances and there are buffer references left over
>> and not immediately cleaned up.
>>
>> Signed-off-by: sfan5<sfan5@live.de>
>> ---
>>   libavcodec/mediacodecdec_common.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/libavcodec/mediacodecdec_common.c b/libavcodec/mediacodecdec_common.c
>> index d6f91e6e89..c888dea8cf 100644
>> --- a/libavcodec/mediacodecdec_common.c
>> +++ b/libavcodec/mediacodecdec_common.c
>> @@ -841,6 +841,18 @@ int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
>>   
>>   int ff_mediacodec_dec_close(AVCodecContext *avctx, MediaCodecDecContext *s)
>>   {
>> +    if (!s)
>> +        return 0;
>> +
>> +    if (s->codec) {
>> +        if (atomic_load(&s->hw_buffer_count) == 0) {
>> +            ff_AMediaCodec_stop(s->codec);
>> +            av_log(avctx, AV_LOG_DEBUG, "MediaCodec %p stopped\n", s->codec);
>> +        } else {
>> +            av_log(avctx, AV_LOG_DEBUG, "Not stopping MediaCodec (there are buffers pending)\n");
>> +        }
>> +    }
>> +
> Could you elaborate on how this resolved the issue?
> If hw_buffer_count is zero, isn’t MediaCodecDecContext will be released immediately?
> If hw_buffer_count isn’t zero, the patch make no real change, so how does this patch work?

Sure.

here's the original report: 
https://github.com/mpv-android/mpv-android/issues/966

summary:

mpv's hwdec code uses a single surface to facilitate zero-copy video 
playback. Keeping an active MediaCodec instance connected to the surface 
blocks the same surface from being used with a different MediaCodec 
instance.

However mpv also keeps a reference to the last rendered frame alive even 
when switching between files. It also flushes the codec when it's done 
with a file. This leads to a situation where hw_buffer_count == 0 && 
refcount > 1.

If you were to say that this should be fixed in mpv I would agree and I 
have indeed proposed a PR for this. But I noticed we're never calling 
stop() in mediacodecdec and it's a very simple remedy.
Zhao Zhili Aug. 9, 2024, 2:17 a.m. UTC | #4 
> On Aug 9, 2024, at 02:14, sfan5 <sfan5@live.de> wrote:
> 
> Am 08.08.24 um 17:29 schrieb Zhao Zhili:
>>> On Aug 8, 2024, at 00:27, sfan5<sfan5@live.de>  wrote:
>>> 
>>> Hi all,
>>> 
>>> attached is a small fix for the MediaCodec code. Tested on Android 14.
>>> 
>>> This can free up vital resources in case of using multiple
>>> decoding instances and there are buffer references left over
>>> and not immediately cleaned up.
>>> 
>>> Signed-off-by: sfan5<sfan5@live.de>
>>> ---
>>>  libavcodec/mediacodecdec_common.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>> 
>>> diff --git a/libavcodec/mediacodecdec_common.c b/libavcodec/mediacodecdec_common.c
>>> index d6f91e6e89..c888dea8cf 100644
>>> --- a/libavcodec/mediacodecdec_common.c
>>> +++ b/libavcodec/mediacodecdec_common.c
>>> @@ -841,6 +841,18 @@ int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
>>>    int ff_mediacodec_dec_close(AVCodecContext *avctx, MediaCodecDecContext *s)
>>>  {
>>> +    if (!s)
>>> +        return 0;
>>> +
>>> +    if (s->codec) {
>>> +        if (atomic_load(&s->hw_buffer_count) == 0) {
>>> +            ff_AMediaCodec_stop(s->codec);
>>> +            av_log(avctx, AV_LOG_DEBUG, "MediaCodec %p stopped\n", s->codec);
>>> +        } else {
>>> +            av_log(avctx, AV_LOG_DEBUG, "Not stopping MediaCodec (there are buffers pending)\n");
>>> +        }
>>> +    }
>>> +
>> Could you elaborate on how this resolved the issue?
>> If hw_buffer_count is zero, isn’t MediaCodecDecContext will be released immediately?
>> If hw_buffer_count isn’t zero, the patch make no real change, so how does this patch work?
> 
> Sure.
> 
> here's the original report: https://github.com/mpv-android/mpv-android/issues/966
> 
> summary:
> 
> mpv's hwdec code uses a single surface to facilitate zero-copy video playback. Keeping an active MediaCodec instance connected to the surface blocks the same surface from being used with a different MediaCodec instance.
> 
> However mpv also keeps a reference to the last rendered frame alive even when switching between files. It also flushes the codec when it's done with a file. This leads to a situation where hw_buffer_count == 0 && refcount > 1.
> 
> If you were to say that this should be fixed in mpv I would agree and I have indeed proposed a PR for this. But I noticed we're never calling stop() in mediacodecdec and it's a very simple remedy.

OK. Please add the background to the commit message. It’s not obvious how to trigger the "hw_buffer_count == 0 && refcount > 1” case.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
diff mbox series

Patch

From 3f5d05920dc6826b4c0ea0ed7969e9259e08084e Mon Sep 17 00:00:00 2001
From: sfan5 <sfan5@live.de>
Date: Wed, 7 Aug 2024 17:48:06 +0200
Subject: [PATCH] avcodec/mediacodecdec: call MediaCodec.stop on close

This can free up vital resources in case of using multiple
decoding instances and there are buffer references left over
and not immediately cleaned up.

Signed-off-by: sfan5 <sfan5@live.de>
---
 libavcodec/mediacodecdec_common.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/libavcodec/mediacodecdec_common.c b/libavcodec/mediacodecdec_common.c
index d6f91e6e89..c888dea8cf 100644
--- a/libavcodec/mediacodecdec_common.c
+++ b/libavcodec/mediacodecdec_common.c
@@ -841,6 +841,18 @@  int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
 
 int ff_mediacodec_dec_close(AVCodecContext *avctx, MediaCodecDecContext *s)
 {
+    if (!s)
+        return 0;
+
+    if (s->codec) {
+        if (atomic_load(&s->hw_buffer_count) == 0) {
+            ff_AMediaCodec_stop(s->codec);
+            av_log(avctx, AV_LOG_DEBUG, "MediaCodec %p stopped\n", s->codec);
+        } else {
+            av_log(avctx, AV_LOG_DEBUG, "Not stopping MediaCodec (there are buffers pending)\n");
+        }
+    }
+
     ff_mediacodec_dec_unref(s);
 
     return 0;
-- 
2.46.0