diff mbox

[FFmpeg-devel] avcodec/atrac3: use AVCodec.init_static_data() to initialize static data

Message ID 20170106013717.1192-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Jan. 6, 2017, 1:37 a.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/atrac3.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Paul B Mahol Jan. 6, 2017, 9:26 a.m. UTC | #1 
On 1/6/17, James Almer <jamrial@gmail.com> wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/atrac3.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/libavcodec/atrac3.c b/libavcodec/atrac3.c
> index 256990b..208762d 100644
> --- a/libavcodec/atrac3.c
> +++ b/libavcodec/atrac3.c
> @@ -771,7 +771,7 @@ static int atrac3_decode_frame(AVCodecContext *avctx,
> void *data,
>      return avctx->block_align;
>  }
>
> -static av_cold void atrac3_init_static_data(void)
> +static av_cold void atrac3_decode_init_static_data(AVCodec *codec)
>  {
>      int i;
>
> @@ -791,7 +791,6 @@ static av_cold void atrac3_init_static_data(void)
>
>  static av_cold int atrac3_decode_init(AVCodecContext *avctx)
>  {
> -    static int static_init_done;
>      int i, ret;
>      int version, delay, samples_per_frame, frame_factor;
>      const uint8_t *edata_ptr = avctx->extradata;
> @@ -802,10 +801,6 @@ static av_cold int atrac3_decode_init(AVCodecContext
> *avctx)
>          return AVERROR(EINVAL);
>      }
>
> -    if (!static_init_done)
> -        atrac3_init_static_data();
> -    static_init_done = 1;
> -
>      /* Take care of the codec-specific extradata. */
>      if (avctx->extradata_size == 14) {
>          /* Parse the extradata, WAV format */
> @@ -932,6 +927,7 @@ AVCodec ff_atrac3_decoder = {
>      .id               = AV_CODEC_ID_ATRAC3,
>      .priv_data_size   = sizeof(ATRAC3Context),
>      .init             = atrac3_decode_init,
> +    .init_static_data = atrac3_decode_init_static_data,
>      .close            = atrac3_decode_close,
>      .decode           = atrac3_decode_frame,
>      .capabilities     = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
> --
> 2.10.2

The issue is that init_static_data is called even when codec is never
gonna be used.
And it is called for both encoder and decoder.

Do we really want such startup speed and memory overhead?
James Almer Jan. 6, 2017, 1:12 p.m. UTC | #2 
On 1/6/2017 6:26 AM, Paul B Mahol wrote:
> On 1/6/17, James Almer <jamrial@gmail.com> wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/atrac3.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/atrac3.c b/libavcodec/atrac3.c
>> index 256990b..208762d 100644
>> --- a/libavcodec/atrac3.c
>> +++ b/libavcodec/atrac3.c
>> @@ -771,7 +771,7 @@ static int atrac3_decode_frame(AVCodecContext *avctx,
>> void *data,
>>      return avctx->block_align;
>>  }
>>
>> -static av_cold void atrac3_init_static_data(void)
>> +static av_cold void atrac3_decode_init_static_data(AVCodec *codec)
>>  {
>>      int i;
>>
>> @@ -791,7 +791,6 @@ static av_cold void atrac3_init_static_data(void)
>>
>>  static av_cold int atrac3_decode_init(AVCodecContext *avctx)
>>  {
>> -    static int static_init_done;
>>      int i, ret;
>>      int version, delay, samples_per_frame, frame_factor;
>>      const uint8_t *edata_ptr = avctx->extradata;
>> @@ -802,10 +801,6 @@ static av_cold int atrac3_decode_init(AVCodecContext
>> *avctx)
>>          return AVERROR(EINVAL);
>>      }
>>
>> -    if (!static_init_done)
>> -        atrac3_init_static_data();
>> -    static_init_done = 1;
>> -
>>      /* Take care of the codec-specific extradata. */
>>      if (avctx->extradata_size == 14) {
>>          /* Parse the extradata, WAV format */
>> @@ -932,6 +927,7 @@ AVCodec ff_atrac3_decoder = {
>>      .id               = AV_CODEC_ID_ATRAC3,
>>      .priv_data_size   = sizeof(ATRAC3Context),
>>      .init             = atrac3_decode_init,
>> +    .init_static_data = atrac3_decode_init_static_data,
>>      .close            = atrac3_decode_close,
>>      .decode           = atrac3_decode_frame,
>>      .capabilities     = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
>> --
>> 2.10.2
> 
> The issue is that init_static_data is called even when codec is never
> gonna be used.
> And it is called for both encoder and decoder.
> 
> Do we really want such startup speed and memory overhead?

Supposedly hardcoded tables builds were added to counter that, at least
partially.

In any case it's not important for me, so this can be dropped if it has
no real advantages. I remember more than one email from people loathing
global state, so I assumed it would be proper to do it this way.
Paul B Mahol Jan. 6, 2017, 1:54 p.m. UTC | #3 
On 1/6/17, James Almer <jamrial@gmail.com> wrote:
> On 1/6/2017 6:26 AM, Paul B Mahol wrote:
>> On 1/6/17, James Almer <jamrial@gmail.com> wrote:
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavcodec/atrac3.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavcodec/atrac3.c b/libavcodec/atrac3.c
>>> index 256990b..208762d 100644
>>> --- a/libavcodec/atrac3.c
>>> +++ b/libavcodec/atrac3.c
>>> @@ -771,7 +771,7 @@ static int atrac3_decode_frame(AVCodecContext *avctx,
>>> void *data,
>>>      return avctx->block_align;
>>>  }
>>>
>>> -static av_cold void atrac3_init_static_data(void)
>>> +static av_cold void atrac3_decode_init_static_data(AVCodec *codec)
>>>  {
>>>      int i;
>>>
>>> @@ -791,7 +791,6 @@ static av_cold void atrac3_init_static_data(void)
>>>
>>>  static av_cold int atrac3_decode_init(AVCodecContext *avctx)
>>>  {
>>> -    static int static_init_done;
>>>      int i, ret;
>>>      int version, delay, samples_per_frame, frame_factor;
>>>      const uint8_t *edata_ptr = avctx->extradata;
>>> @@ -802,10 +801,6 @@ static av_cold int atrac3_decode_init(AVCodecContext
>>> *avctx)
>>>          return AVERROR(EINVAL);
>>>      }
>>>
>>> -    if (!static_init_done)
>>> -        atrac3_init_static_data();
>>> -    static_init_done = 1;
>>> -
>>>      /* Take care of the codec-specific extradata. */
>>>      if (avctx->extradata_size == 14) {
>>>          /* Parse the extradata, WAV format */
>>> @@ -932,6 +927,7 @@ AVCodec ff_atrac3_decoder = {
>>>      .id               = AV_CODEC_ID_ATRAC3,
>>>      .priv_data_size   = sizeof(ATRAC3Context),
>>>      .init             = atrac3_decode_init,
>>> +    .init_static_data = atrac3_decode_init_static_data,
>>>      .close            = atrac3_decode_close,
>>>      .decode           = atrac3_decode_frame,
>>>      .capabilities     = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
>>> --
>>> 2.10.2
>>
>> The issue is that init_static_data is called even when codec is never
>> gonna be used.
>> And it is called for both encoder and decoder.
>>
>> Do we really want such startup speed and memory overhead?
>
> Supposedly hardcoded tables builds were added to counter that, at least
> partially.
>
> In any case it's not important for me, so this can be dropped if it has
> no real advantages. I remember more than one email from people loathing
> global state, so I assumed it would be proper to do it this way.

I have no real opinion on this, but maybe others do have?

We could add smarter init_static_data...
Nicolas George Jan. 6, 2017, 1:59 p.m. UTC | #4 
Le septidi 17 nivôse, an CCXXV, Paul B Mahol a écrit :
> I have no real opinion on this, but maybe others do have?
> 
> We could add smarter init_static_data...

Changing the framework to call init_static_data only the first time the
codec is ever opened seems like the obvious thing to do. It requires an
atomic flag per codec, I think we can devise a way.

By the way, the sine table generator in asrc_sine is significantly
faster than the float version from the libc, and bit-exact with itself
to boot, maybe you can make use of it.

Regards,
Michael Niedermayer Jan. 6, 2017, 2:39 p.m. UTC | #5 
On Fri, Jan 06, 2017 at 02:54:42PM +0100, Paul B Mahol wrote:
> On 1/6/17, James Almer <jamrial@gmail.com> wrote:
> > On 1/6/2017 6:26 AM, Paul B Mahol wrote:
> >> On 1/6/17, James Almer <jamrial@gmail.com> wrote:
> >>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>> ---
> >>>  libavcodec/atrac3.c | 8 ++------
> >>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/libavcodec/atrac3.c b/libavcodec/atrac3.c
> >>> index 256990b..208762d 100644
> >>> --- a/libavcodec/atrac3.c
> >>> +++ b/libavcodec/atrac3.c
> >>> @@ -771,7 +771,7 @@ static int atrac3_decode_frame(AVCodecContext *avctx,
> >>> void *data,
> >>>      return avctx->block_align;
> >>>  }
> >>>
> >>> -static av_cold void atrac3_init_static_data(void)
> >>> +static av_cold void atrac3_decode_init_static_data(AVCodec *codec)
> >>>  {
> >>>      int i;
> >>>
> >>> @@ -791,7 +791,6 @@ static av_cold void atrac3_init_static_data(void)
> >>>
> >>>  static av_cold int atrac3_decode_init(AVCodecContext *avctx)
> >>>  {
> >>> -    static int static_init_done;
> >>>      int i, ret;
> >>>      int version, delay, samples_per_frame, frame_factor;
> >>>      const uint8_t *edata_ptr = avctx->extradata;
> >>> @@ -802,10 +801,6 @@ static av_cold int atrac3_decode_init(AVCodecContext
> >>> *avctx)
> >>>          return AVERROR(EINVAL);
> >>>      }
> >>>
> >>> -    if (!static_init_done)
> >>> -        atrac3_init_static_data();
> >>> -    static_init_done = 1;
> >>> -
> >>>      /* Take care of the codec-specific extradata. */
> >>>      if (avctx->extradata_size == 14) {
> >>>          /* Parse the extradata, WAV format */
> >>> @@ -932,6 +927,7 @@ AVCodec ff_atrac3_decoder = {
> >>>      .id               = AV_CODEC_ID_ATRAC3,
> >>>      .priv_data_size   = sizeof(ATRAC3Context),
> >>>      .init             = atrac3_decode_init,
> >>> +    .init_static_data = atrac3_decode_init_static_data,
> >>>      .close            = atrac3_decode_close,
> >>>      .decode           = atrac3_decode_frame,
> >>>      .capabilities     = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
> >>> --
> >>> 2.10.2
> >>
> >> The issue is that init_static_data is called even when codec is never
> >> gonna be used.
> >> And it is called for both encoder and decoder.
> >>
> >> Do we really want such startup speed and memory overhead?
> >
> > Supposedly hardcoded tables builds were added to counter that, at least
> > partially.
> >
> > In any case it's not important for me, so this can be dropped if it has
> > no real advantages. I remember more than one email from people loathing
> > global state, so I assumed it would be proper to do it this way.
> 
> I have no real opinion on this, but maybe others do have?
> 

> We could add smarter init_static_data...

+1


[...]
diff mbox

Patch

diff --git a/libavcodec/atrac3.c b/libavcodec/atrac3.c
index 256990b..208762d 100644
--- a/libavcodec/atrac3.c
+++ b/libavcodec/atrac3.c
@@ -771,7 +771,7 @@  static int atrac3_decode_frame(AVCodecContext *avctx, void *data,
     return avctx->block_align;
 }
 
-static av_cold void atrac3_init_static_data(void)
+static av_cold void atrac3_decode_init_static_data(AVCodec *codec)
 {
     int i;
 
@@ -791,7 +791,6 @@  static av_cold void atrac3_init_static_data(void)
 
 static av_cold int atrac3_decode_init(AVCodecContext *avctx)
 {
-    static int static_init_done;
     int i, ret;
     int version, delay, samples_per_frame, frame_factor;
     const uint8_t *edata_ptr = avctx->extradata;
@@ -802,10 +801,6 @@  static av_cold int atrac3_decode_init(AVCodecContext *avctx)
         return AVERROR(EINVAL);
     }
 
-    if (!static_init_done)
-        atrac3_init_static_data();
-    static_init_done = 1;
-
     /* Take care of the codec-specific extradata. */
     if (avctx->extradata_size == 14) {
         /* Parse the extradata, WAV format */
@@ -932,6 +927,7 @@  AVCodec ff_atrac3_decoder = {
     .id               = AV_CODEC_ID_ATRAC3,
     .priv_data_size   = sizeof(ATRAC3Context),
     .init             = atrac3_decode_init,
+    .init_static_data = atrac3_decode_init_static_data,
     .close            = atrac3_decode_close,
     .decode           = atrac3_decode_frame,
     .capabilities     = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,