diff mbox

[FFmpeg-devel,1/3,v2] avcodec/util: use a mutex instead of atomics in avcodec_register()

Message ID 20180105014044.2132-1-jamrial@gmail.com
State Accepted
Commit 9ed4ebc530dde681943389b1f97db90b546ad38d
Headers show

Commit Message

James Almer Jan. 5, 2018, 1:40 a.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/utils.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

wm4 Jan. 5, 2018, 4:02 p.m. UTC | #1 
On Thu,  4 Jan 2018 22:40:44 -0300
James Almer <jamrial@gmail.com> wrote:

> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/utils.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index dfbfe98d63..4d736d2e7d 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -26,7 +26,6 @@
>   */
>  
>  #include "config.h"
> -#include "libavutil/atomic.h"
>  #include "libavutil/attributes.h"
>  #include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
> @@ -127,17 +126,24 @@ int av_codec_is_decoder(const AVCodec *codec)
>      return codec && (codec->decode || codec->receive_frame);
>  }
>  
> +static AVMutex codec_register_mutex = AV_MUTEX_INITIALIZER;
> +
>  av_cold void avcodec_register(AVCodec *codec)
>  {
>      AVCodec **p;
>      avcodec_init();
> +
> +    ff_mutex_lock(&codec_register_mutex);
>      p = last_avcodec;
> -    codec->next = NULL;
>  
> -    while(*p || avpriv_atomic_ptr_cas((void * volatile *)p, NULL, codec))
> +    while (*p)
>          p = &(*p)->next;
> +    *p          = codec;
> +    codec->next = NULL;
>      last_avcodec = &codec->next;
>  
> +    ff_mutex_unlock(&codec_register_mutex);
> +
>      if (codec->init_static_data)
>          codec->init_static_data(codec);
>  }

Seems good, but should init_static_data really be outside of the lock?
(Sure, it wasn't under a look before either, but still.)
James Almer Jan. 5, 2018, 4:18 p.m. UTC | #2 
On 1/5/2018 1:02 PM, wm4 wrote:
> On Thu,  4 Jan 2018 22:40:44 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/utils.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index dfbfe98d63..4d736d2e7d 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -26,7 +26,6 @@
>>   */
>>  
>>  #include "config.h"
>> -#include "libavutil/atomic.h"
>>  #include "libavutil/attributes.h"
>>  #include "libavutil/avassert.h"
>>  #include "libavutil/avstring.h"
>> @@ -127,17 +126,24 @@ int av_codec_is_decoder(const AVCodec *codec)
>>      return codec && (codec->decode || codec->receive_frame);
>>  }
>>  
>> +static AVMutex codec_register_mutex = AV_MUTEX_INITIALIZER;
>> +
>>  av_cold void avcodec_register(AVCodec *codec)
>>  {
>>      AVCodec **p;
>>      avcodec_init();
>> +
>> +    ff_mutex_lock(&codec_register_mutex);
>>      p = last_avcodec;
>> -    codec->next = NULL;
>>  
>> -    while(*p || avpriv_atomic_ptr_cas((void * volatile *)p, NULL, codec))
>> +    while (*p)
>>          p = &(*p)->next;
>> +    *p          = codec;
>> +    codec->next = NULL;
>>      last_avcodec = &codec->next;
>>  
>> +    ff_mutex_unlock(&codec_register_mutex);
>> +
>>      if (codec->init_static_data)
>>          codec->init_static_data(codec);
>>  }
> 
> Seems good, but should init_static_data really be outside of the lock?
> (Sure, it wasn't under a look before either, but still.)

I don't think calling avcodec_register() twice with the same AVCodec is
defined or supported at all, so outside of creating the linked list i
don't see any other potential race here. But i may be wrong.

In any case, that's something for a separate patch as it's a change in
behavior (and may make things slower), so pushed as is.
diff mbox

Patch

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index dfbfe98d63..4d736d2e7d 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -26,7 +26,6 @@ 
  */
 
 #include "config.h"
-#include "libavutil/atomic.h"
 #include "libavutil/attributes.h"
 #include "libavutil/avassert.h"
 #include "libavutil/avstring.h"
@@ -127,17 +126,24 @@  int av_codec_is_decoder(const AVCodec *codec)
     return codec && (codec->decode || codec->receive_frame);
 }
 
+static AVMutex codec_register_mutex = AV_MUTEX_INITIALIZER;
+
 av_cold void avcodec_register(AVCodec *codec)
 {
     AVCodec **p;
     avcodec_init();
+
+    ff_mutex_lock(&codec_register_mutex);
     p = last_avcodec;
-    codec->next = NULL;
 
-    while(*p || avpriv_atomic_ptr_cas((void * volatile *)p, NULL, codec))
+    while (*p)
         p = &(*p)->next;
+    *p          = codec;
+    codec->next = NULL;
     last_avcodec = &codec->next;
 
+    ff_mutex_unlock(&codec_register_mutex);
+
     if (codec->init_static_data)
         codec->init_static_data(codec);
 }