[FFmpeg-devel,1/7] avcodec/mediacodec: convert to stdatomic

Le 23 mars 2017 12:35 AM, "James Almer" <jamrial@gmail.com> a écrit :

Signed-off-by: James Almer <jamrial@gmail.com>
---
Untested.

 libavcodec/mediacodec.c           |  5 ++---
 libavcodec/mediacodecdec.c        |  1 -
 libavcodec/mediacodecdec_common.c | 14 ++++++--------
 libavcodec/mediacodecdec_common.h |  5 +++--
 4 files changed, 11 insertions(+), 14 deletions(-)


The underlying buffer is meant to be released once. Unless i miss something
there, you could potentially release a later buffer using the same index or
release an invalid buffer. A mediacodec buffer can only be released once.
Once it is released it goes back to the decoder pool and it is not usable
anymore unless it bas been returned by the decoder again. The buffer itself
is represented by an index. Can you bring the original check back ?
(released == 1)

I dont have the time to review and test the entire patch (nor the machine,
i m writing from my phone) as i am on holidays but i ll be back on monday
(and i ll test the patch).

Matthieu
[...]

On 3/23/2017 3:15 PM, Matthieu Bouron wrote:
> Le 23 mars 2017 12:35 AM, "James Almer" <jamrial@gmail.com> a écrit :
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Untested.
> 
>  libavcodec/mediacodec.c           |  5 ++---
>  libavcodec/mediacodecdec.c        |  1 -
>  libavcodec/mediacodecdec_common.c | 14 ++++++--------
>  libavcodec/mediacodecdec_common.h |  5 +++--
>  4 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/libavcodec/mediacodec.c b/libavcodec/mediacodec.c
> index 4ad5921bc2..91f725621d 100644
> --- a/libavcodec/mediacodec.c
> +++ b/libavcodec/mediacodec.c
> @@ -31,7 +31,6 @@
>  #include <jni.h>
> 
>  #include "libavcodec/avcodec.h"
> -#include "libavutil/atomic.h"
>  #include "libavutil/mem.h"
> 
>  #include "ffjni.h"
> @@ -90,9 +89,9 @@ void av_mediacodec_default_free(AVCodecContext *avctx)
>  int av_mediacodec_release_buffer(AVMediaCodecBuffer *buffer, int render)
>  {
>      MediaCodecDecContext *ctx = buffer->ctx;
> -    int released = avpriv_atomic_int_add_and_fetch(&buffer->released, 1);
> +    atomic_int released = atomic_fetch_add(&buffer->released, 1);
> 
> -    if (released == 1) {
> +    if (!released) {
> 
> 
> Hello,
> 
> The underlying buffer is meant to be released once. Unless i miss something
> there, you could potentially release a later buffer using the same index or
> release an invalid buffer. A mediacodec buffer can only be released once.
> Once it is released it goes back to the decoder pool and it is not usable
> anymore unless it bas been returned by the decoder again. The buffer itself
> is represented by an index. Can you bring the original check back ?
> (released == 1)

C11 atomics only has fetch_add functions, not add_fetch like the previous
implementation.

add_fetch(&dst, 1) adds 1 to dst, then returns the new dst value.
fetch_add(&dst, 1) adds 1 to dst, then returns the old dst value.

This means that for us to check if the buffer was released once, while for
add_fetch we had to check if the return value was 1, for fetch_add we need
to check if it's zero.

> 
> I dont have the time to review and test the entire patch (nor the machine,
> i m writing from my phone) as i am on holidays but i ll be back on monday
> (and i ll test the patch).
> 
> Matthieu
> [...]
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Le 23 mars 2017 7:31 PM, "James Almer" <jamrial@gmail.com> a écrit :

On 3/23/2017 3:15 PM, Matthieu Bouron wrote:
> Le 23 mars 2017 12:35 AM, "James Almer" <jamrial@gmail.com> a écrit :
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Untested.
>
>  libavcodec/mediacodec.c           |  5 ++---
>  libavcodec/mediacodecdec.c        |  1 -
>  libavcodec/mediacodecdec_common.c | 14 ++++++--------
>  libavcodec/mediacodecdec_common.h |  5 +++--
>  4 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/libavcodec/mediacodec.c b/libavcodec/mediacodec.c
> index 4ad5921bc2..91f725621d 100644
> --- a/libavcodec/mediacodec.c
> +++ b/libavcodec/mediacodec.c
> @@ -31,7 +31,6 @@
>  #include <jni.h>
>
>  #include "libavcodec/avcodec.h"
> -#include "libavutil/atomic.h"
>  #include "libavutil/mem.h"
>
>  #include "ffjni.h"
> @@ -90,9 +89,9 @@ void av_mediacodec_default_free(AVCodecContext *avctx)
>  int av_mediacodec_release_buffer(AVMediaCodecBuffer *buffer, int render)
>  {
>      MediaCodecDecContext *ctx = buffer->ctx;
> -    int released = avpriv_atomic_int_add_and_fetch(&buffer->released, 1);
> +    atomic_int released = atomic_fetch_add(&buffer->released, 1);
>
> -    if (released == 1) {
> +    if (!released) {
>
>
> Hello,
>
> The underlying buffer is meant to be released once. Unless i miss
something
> there, you could potentially release a later buffer using the same index
or
> release an invalid buffer. A mediacodec buffer can only be released once.
> Once it is released it goes back to the decoder pool and it is not usable
> anymore unless it bas been returned by the decoder again. The buffer
itself
> is represented by an index. Can you bring the original check back ?
> (released == 1)

C11 atomics only has fetch_add functions, not add_fetch like the previous
implementation.

add_fetch(&dst, 1) adds 1 to dst, then returns the new dst value.
fetch_add(&dst, 1) adds 1 to dst, then returns the old dst value.

This means that for us to check if the buffer was released once, while for
add_fetch we had to check if the return value was 1, for fetch_add we need
to check if it's zero.


Thanks for the explanation and for the patch (and sorry for the noise).

Matthieu

[...]