[FFmpeg-devel] [PATCH] Add ATRAC3+ decoder

[Reimar Döffinger]

On 14.10.2013, at 22:09, Michael Niedermayer <michaelni at gmx.at> wrote:

> On Mon, Oct 14, 2013 at 08:52:04PM +0200, Maxim Polijakowski wrote:
>> 
>>>> +    const uint8_t *buf  = avpkt->data;
>>>> +    int buf_size        = avpkt->size;
>>>> +    ATRAC3PContext *ctx = avctx->priv_data;
>>>> +    AVFrame *frame      = data;
>>>> +    int i, ret, ch_unit_id, ch_block = 0, out_ch_index = 0, channels_to_process;
>>>> +    float **samples_p = (float **)frame->extended_data;
>>>> +
>>>> +    frame->nb_samples = ATRAC3P_FRAME_SAMPLES;
>>>> +    if ((ret = ff_get_buffer(avctx, frame, 0)) < 0) {
>>>> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    init_get_bits(&ctx->gb, buf, buf_size * 8);
>>> if ((ret = init_get_bits8(&ctx->gb, avpkt->data, avpkt->size)) < 0)
>>>    return ret;
>> 
>> Done.
>> 
>>> [...]
>>> 
>>>> +
>>>> +/**
>>>> + *  @file
>>>> + *  DSP functions for ATRAC3+ decoder.
>>>> + */
>>>> +
>>>> +#include <math.h>
>>>> +
>>>> +#include "libavutil/float_dsp.h"
>>>> +#include "avcodec.h"
>>>> +#include "sinewin.h"
>>>> +#include "fft.h"
>>>> +#include "atrac3plus.h"
>>>> +#include "atrac3plus_data.h"
>>>> +
>>>> +#define ATRAC3P_MDCT_SIZE (ATRAC3P_SUBBAND_SAMPLES * 2)
>>>> +
>>>> +static AVFloatDSPContext atrac3p_dsp;
>>> This looks strange and is probably incorrect, it probably should be in
>>> private context.
>>> 
>> 
>> What is wrong with that?
> 
> Initialization uses a local per instance flag
> (avctx->flags & CODEC_FLAG_BITEXACT)
> thus if there are 2 instances, one with the flag set and one not set
> a single global context could only be corrct for both if the flag
> has no effect and would be unused

Note that this is just an example, such DSP contexts will also depend on e.g. CPU flags,
which can be set per context as well as far as I know.
So using a global variable will break a whole host of things and possibly cause hard to debug issues later.
And it's not just that these flags get ignored (which would be somewhat reasonable), but there is even a risk of race conditions in these cases, e.g. initializing another codec with bitexact might replace the function pointers in the middle of execution for another one that was initialized without, making it change its output.