[FFmpeg-devel] [PATCH] Newtek SpeedHQ decoder.

[Steinar H. Gunderson]

On Sun, Jan 08, 2017 at 01:45:07PM +0100, Paul B Mahol wrote:
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index ca8b786077..23e1ecc7a7 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -388,6 +388,14 @@ enum AVCodecID {
>>      AV_CODEC_ID_DXV,
>>      AV_CODEC_ID_SCREENPRESSO,
>>      AV_CODEC_ID_RSCC,
>> +    AV_CODEC_ID_SHQ0,
>> +    AV_CODEC_ID_SHQ1,
>> +    AV_CODEC_ID_SHQ2,
>> +    AV_CODEC_ID_SHQ3,
>> +    AV_CODEC_ID_SHQ4,
>> +    AV_CODEC_ID_SHQ5,
>> +    AV_CODEC_ID_SHQ7,
>> +    AV_CODEC_ID_SHQ9,
> Wrong place, put it after YLC, bellow.

Will move. What's the intended ordering here, really?

>> --- a/libavcodec/bitstream.c
>> +++ b/libavcodec/bitstream.c
>> @@ -126,14 +126,6 @@ static int alloc_table(VLC *vlc, int size, int
>> use_static)
>>      return index;
>>  }
>>
>> -static av_always_inline uint32_t bitswap_32(uint32_t x)
>> -{
>> -    return (uint32_t)ff_reverse[ x        & 0xFF] << 24 |
>> -           (uint32_t)ff_reverse[(x >> 8)  & 0xFF] << 16 |
>> -           (uint32_t)ff_reverse[(x >> 16) & 0xFF] << 8  |
>> -           (uint32_t)ff_reverse[ x >> 24];
>> -}
>> -
> Huh?!? This move should be separate commit.

It's not a removal; it's a move. I need access to bitswap_32() to reverse
bits, so I thought I should move it around rather than duplicate it. Should
it still go into a pre-patch?

>> +        .id        = AV_CODEC_ID_SHQ0,
>> +        .type      = AVMEDIA_TYPE_VIDEO,
>> +        .name      = "speedhq",
>> +        .long_name = NULL_IF_CONFIG_SMALL("NewTek SpeedHQ 4:2:0"),
>> +        .props     = AV_CODEC_PROP_LOSSY,
> Missing, INTRA_ONLY flag. Here and all bellow.

Will fix. Combining also with the previous request to get only one decoder.

>> +    rbuf = av_malloc(buf_size + AV_INPUT_BUFFER_PADDING_SIZE);
>> +    if (!rbuf) {
>> +        av_log(avctx, AV_LOG_ERROR, "Cannot allocate temporary buffer\n");
>> +        av_free(rbuf);
>> +        return AVERROR(ENOMEM);
>> +    }
>> +    memcpy(rbuf, buf, buf_size);
>> +    memset(rbuf + buf_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> Huh?!? Is this needed at all?

Anything else will cause Valgrind hits when you reach the end of the
bitstream (and nondeterministic behavior on truncated bitstream),
so this seems cheap for what it gains. Other codecs are already doing the
same, AFAIK.

> Do we really need this many codec ids?
> Is format somehow signaled in bitstream or its only available via codec tag?

I've moved to a single codec ID. There's no signal in bitstream; it's codec
tag only.

/* Steinar */
-- 
Homepage: https://www.sesse.net/