[FFmpeg-devel] [PATCHv2] avcodec/siren: add checksum calculation

[Peter Ross]

On Thu, Sep 09, 2021 at 10:40:06AM -0300, James Almer wrote:
> On 9/9/2021 5:46 AM, Peter Ross wrote:
> > ---
> > 
> > Thanks for suggestion. I will apply in a couple days if no objections.
> > 
> >   libavcodec/siren.c | 32 +++++++++++++++++++++++++++++++-
> >   1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/siren.c b/libavcodec/siren.c
> > index 2161b29a2c..40910f34e5 100644
> > --- a/libavcodec/siren.c
> > +++ b/libavcodec/siren.c
> > @@ -752,7 +752,37 @@ static int siren_decode(AVCodecContext *avctx, void *data,
> >               frame_error = 1;
> >       }
> > -    skip_bits(gb, s->checksum_bits);
> > +    if ((avctx->err_recognition & AV_EF_CRCCHECK) && s->checksum_bits) {
> > +        static const uint16_t ChecksumTable[4] = {0x7F80, 0x7878, 0x6666, 0x5555};
> > +        int wpf, checksum, sum, calculated_checksum, temp1, temp2;
> > +
> > +        wpf = bits_per_frame / 16;
> > +
> > +        checksum = AV_RB16(avpkt->data + (wpf - 1) * 2) & ((1 << s->checksum_bits) - 1);
> 
> Shouldn't you read this value from the getbitcontext?

yes, but...

decode_vector() infrequently overreads the getbitcontext, meaning we don't always have
the four checkbits left at the end.

the overreads also happen in the decoder library on which the FFmpeg siren codec is based
(https://github.com/kakaroto/libsiren), and are ignored there too.

i can fix this by putting 'get_bits_left(gb) - s->checksum_bits' guards throughout the
decoder. i guess that's that i will end up doing.

> > +        sum = 0;
> > +        for (int i = 0; i < wpf - 1; i++)
> > +            sum ^= AV_RB16(avpkt->data + i * 2) << (i % 15);
> > +        sum ^= (AV_RB16(avpkt->data + (wpf - 1) * 2) & ~checksum) << ((wpf - 1) % 15);
> > +        sum = (sum >> 15) ^ (sum & 0x7FFF);
> > +
> > +        calculated_checksum = 0;
> > +        for (int i = 0; i < 4; i++) {
> > +            temp1 = ChecksumTable[i] & sum;
> > +            for (int j = 8; j > 0; j >>= 1) {
> > +                temp2 = temp1 >> j;
> > +                temp1 ^= temp2;
> > +            }
> > +            calculated_checksum <<= 1;
> > +            calculated_checksum |= temp1 & 1;
> > +        }
> 
> What crc is this? If it's not already supported by AVCRC, could it be
> implemented?

no idea. it does not look like anything we have in avcrc.

the siren7 codec specification does not describe the checksum.
this appears to be added in the microsoft implementation of siren.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210910/f1a993ee/attachment.sig>