[FFmpeg-devel] [PATCH] ALS decoder

[Thilo Borgmann]

Michael Niedermayer schrieb:
> On Fri, Sep 04, 2009 at 09:48:46PM +0200, Thilo Borgmann wrote:
>> Michael Niedermayer schrieb:
>>> On Thu, Sep 03, 2009 at 09:44:25PM +0200, Thilo Borgmann wrote:
>>>> Revision 16 attached.
> 
> [...]
> 
> 
>>
>>> [...]
>>>> +typedef struct {
>>>> +    int resolution;           ///< 000 = 8-bit; 001 = 16-bit; 010 = 24-bit; 011 = 32-bit
>>>> +    int floating;             ///< 1 = IEEE 32-bit floating-point, 0 = integer
>>>> +    int frame_length;         ///< frame length for each frame (last frame may differ)
>>>> +    int ra_distance;          ///< distance between RA frames (in frames, 0...255)
>>> if each frame is a RA frame that would be 1 i  assume, but then 0...255
>>> makes no sense as 0 makes no sense
>> Makes sense. If ra_distance == 0, there are no RA frames at all.
> 
> ra frames are keyframes, can the decoder even start decoding on non ra
> frames? is this even allowed by the spec? and well defined?
> 
> 
> [...]
> 
>>
>>> [...]
>>>> +/** Decodes an ALS frame.
>>>> + */
>>>> +static int decode_frame(AVCodecContext *avctx,
>>>> +                        void *data, int *data_size,
>>>> +                        AVPacket *avpkt)
>>>> +{
>>>> +    ALSDecContext *ctx       = avctx->priv_data;
>>>> +    ALSSpecificConfig *sconf = &ctx->sconf;
>>>> +    const uint8_t *buffer    = avpkt->data;
>>>> +    int buffer_size          = avpkt->size;
>>>> +    int invalid_frame, size;
>>>> +    unsigned int c, sample, ra_frame, bytes_read, shift;
>>>> +
>>>> +    init_get_bits(&ctx->gb, buffer, buffer_size * 8);
>>>> +    ra_frame = sconf->ra_distance && !(ctx->frame_id % sconf->ra_distance);
>>> is it true that if ra_distance == 0 then no frame is a ra frame?
>>> seems odd
>>>
>> As mentioned above, this is the specified behaviour.
>> And that is the reason for using av_mallocz() for the raw_buffer buffer,
>> to have fake 0's for prediction in the carryover samples from frame "-1".
> 
> does the spec say they should be 0 ?
> 

This whole case is not mentioned explicitly in the spec (as fas as I can
see). But this is produced by the reference encoder and can be followed
implicitly - before the first frame, there is silence (0). Until we
figured that out, we had some "randomly" broken files because of
non-zero memory chunks allocated for the first carryover buffer.

If we would not implicitly assume this from the specs and treat the very
first frame to be a RA frame (keyframe) in all cases, decoding fails.


>>
>>
>>
>>>> +    if (sconf->bgmc) {
>>>> +        // TODO: BGMC mode
>>>> +    } else {
>>>> +        s[0] = get_bits(gb, 4 + (sconf->resolution > 1));
>>>> +        for (k = 1; k < sub_blocks; k++)
>>>> +            s[k] = s[k - 1] + decode_rice(gb, 0);
>>>> +    }
>>>> +
>>>> +    if (get_bits1(gb))
>>>> +        *shift_lsbs = get_bits(gb, 4) + 1;
>>>> +
>>>> +
>>>> +    if (!sconf->rlslms) {
>>>> +        if (sconf->adapt_order) {
>>>> +            int opt_order_length = FFMAX(av_ceil_log2((block_length >> 3) - 1), 1);
>>>> +            opt_order_length     = FFMIN(av_ceil_log2(sconf->max_order+1), opt_order_length);
>>> this can be calculated as
>>> av_ceil_log2(av_clip(...))
>>>
>>> which should be simpler and faster
>> Justin mentioned this during his GSoC review and we agreed that this is
>> not an av_clip() case. I could not find an equal solution, can you?
> 
> a quick try: (entirely untested, might contain errors)
> 
> a = FFMAX(av_ceil_log2((block_length >> 3) - 1), 1);
> opt_order_length = FFMIN(av_ceil_log2(sconf->max_order+1), a);
> 
> a = FFMAX((block_length >> 3) - 1, 2);
> opt_order_length = av_ceil_log2(FFMIN(sconf->max_order+1, a));
> 
> opt_order_length = av_ceil_log2(FFMIN(sconf->max_order+1, FFMAX((block_length >> 3) - 1, 2)));
> 
> opt_order_length = av_ceil_log2(FFMIN(sconf->max_order+1, FFMAX((block_length >> 3) - 1, 2)));
> 
> opt_order_length = av_ceil_log2(av_clip((block_length >> 3) - 1, 2, sconf->max_order+1))

This can be assumed by the values we see here, but mathematically this
is not equal - a block_length == 16 proves this wrong (opt_order_length
== 1 which is not possible in the av_clip case).

Also the specs explicitly shows a block_length == 16 case.


>>
>>
>>>> +    // reconstruct all samples from residuals
>>>> +    if (ra_block) {
>>>> +        unsigned int progressive = FFMIN(block_length, opt_order);
>>> useless, you can just use opt_order instead of progressive in te following
>>> code
>> If block_length < opt_order, max would have a wrong value and y would
>> not be correct any more.
> 
> nonsense
> if block_length < opt_order, smp < block_length -> smp < opt_order -> 
> dequant = 1 -> max = smp
> 
> 
>> I think the specs don't say anything about it but I think this weird
>> case might be true for small last frames + block switching + high
>> prediction order. The reference decoder does this, too.
> 
> the reference is of no relevance, anything that is equivalent is correct,
> actually anything that follows the spec is correct even if its not equivalent
> to what the reference does.
> but we dont need that reasoning here anyway ...

You're right here, opt_order will be used only in the next revision :)

Thanks!

-Thilo