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)

Attachment: signature.asc
Description: PGP signature

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to