[FFmpeg-devel] [PATCH 1/3] avcodec: add siren audio decoder

[James Almer]

On 2/19/2020 1:50 PM, Paul B Mahol wrote:
> On 2/19/20, James Almer <jamrial at gmail.com> wrote:
>> On 2/19/2020 7:30 AM, Paul B Mahol wrote:
>>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>>> +static int siren_decode(AVCodecContext *avctx, void *data,
>>> +                        int *got_frame, AVPacket *avpkt)
>>> +{
>>> +    SirenContext *s = avctx->priv_data;
>>> +    GetBitContext *gb = &s->gb;
>>> +    AVFrame *frame = data;
>>> +    int ret, number_of_valid_coefs = 20 * s->number_of_regions;
>>> +    int frame_error = 0, rate_control = 0;
>>> +
>>> +    if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0)
>>> +        return ret;
>>> +
>>> +    decode_envelope(s, gb, s->number_of_regions,
>>> +                    s->decoder_standard_deviation,
>>> +                    s->absolute_region_power_index, s->esf_adjustment);
>>> +
>>> +    rate_control = get_bits(gb, 4);
>>> +
>>> +    categorize_regions(s->number_of_regions, get_bits_left(gb),
>>> +                       s->absolute_region_power_index,
>>> s->power_categories,
>>> +                       s->category_balance);
>>> +
>>> +    for (int i = 0; i < rate_control; i++) {
>>> +        s->power_categories[s->category_balance[i]]++;
>>> +    }
>>> +
>>> +    ret = decode_vector(s, s->number_of_regions, get_bits_left(gb),
>>> +                        s->decoder_standard_deviation,
>>> s->power_categories,
>>> +                        s->imdct_in, s->scale_factor);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    if (get_bits_left(gb) > 0) {
>>> +        do {
>>> +            if (!get_bits1(gb))
>>> +                frame_error = 1;
>>> +        } while (get_bits_left(gb) > 0);
>>> +    } else if (get_bits_left(gb) < 0 &&
>>> +               rate_control + 1 < s->rate_control_possibilities) {
>>> +        frame_error |= 2;
>>> +    }
>>> +
>>> +    for (int i = 0; i < s->number_of_regions; i++) {
>>> +        if (s->absolute_region_power_index[i] > 33 ||
>>> +            s->absolute_region_power_index[i] < -31)
>>> +            frame_error |= 4;
>>> +    }
>>> +
>>> +    if (frame_error) {
>>
>> You're not really doing anything different depending on the kind of
>> error you saw above, so may as well just make frame_error 0 or 1.
> 
> DO not like that.
> 
>>
>> Also, you may want to abort decoding if err_detect is set to explode.
> 
> DO not like that.
> 
>>
>>> +        for (int i = 0; i < number_of_valid_coefs; i++) {
>>> +            s->imdct_in[i] = s->backup_frame[i];
>>> +            s->backup_frame[i] = 0;
>>> +        }
>>> +    } else {
>>> +        for (int i = 0; i < number_of_valid_coefs; i++)
>>> +            s->backup_frame[i] = s->imdct_in[i];
>>> +    }
>>> +
>>> +    frame->nb_samples = FRAME_SIZE;
>>> +    if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
>>> +        return ret;
>>> +
>>> +    for (int i = 0; i < 320; i += 2)
>>> +        s->imdct_in[i] *= -1;
>>> +
>>> +    s->tx_fn(s->tx_ctx, s->imdct_out, s->imdct_in, sizeof(float));
>>> +    s->fdsp->vector_fmul_window((float *)frame->data[0],
>>> +                                s->imdct_prev + (FRAME_SIZE >> 1),
>>> +                                s->imdct_out, s->window,
>>> +                                FRAME_SIZE >> 1);
>>> +    FFSWAP(float *, s->imdct_out, s->imdct_prev);
>>
>> Instead of this, can't you keep an AVFrame reference in SirenContext and
>> use ff_reget_buffer()? With or without read only flag depending on
>> frame_error.
> 
> DO not like that.
> 
>>
>>> +
>>> +    *got_frame = 1;
>>> +
>>> +    return avpkt->size;
>>> +}
>>> +
>>> +static av_cold int siren_close(AVCodecContext *avctx)
>>> +{
>>> +    SirenContext *s = avctx->priv_data;
>>> +
>>> +    av_freep(&s->fdsp);
>>> +    av_tx_uninit(&s->tx_ctx);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +AVCodec ff_siren_decoder = {
>>> +    .name           = "siren",
>>> +    .long_name      = NULL_IF_CONFIG_SMALL("Siren"),
>>> +    .priv_data_size = sizeof(SirenContext),
>>> +    .type           = AVMEDIA_TYPE_AUDIO,
>>> +    .id             = AV_CODEC_ID_SIREN,
>>> +    .init           = siren_init,
>>> +    .close          = siren_close,
>>> +    .decode         = siren_decode,
>>
>> Missing a flush() function to clear s->backup_frame, s->imdct_in,
>> s->imdct_out and s->imdct_prev.
> 
> DO not like that.
>>
>>> +    .capabilities   = AV_CODEC_CAP_DR1,
>>
>> Also init thread safe caps_internal.
> DO not like that.

At some point you'll have to realize there's a difference between being
funny and being insufferable.