[FFmpeg-devel] [PATCH v3 2/2] Newtek SpeedHQ decoder.

Steinar H. Gunderson steinar+ffmpeg at gunderson.no
Mon Jan 9 19:36:40 EET 2017


On Mon, Jan 09, 2017 at 06:30:52PM +0100, Paul B Mahol wrote:
>> +/* AC codes: Very similar but not identical to MPEG-2. */
>> +static uint16_t speedhq_vlc[123][2] = {
> Can this be uint8_t too?

No, it goes into an RLTable, which expects an uint16_t*. Besides, after
bit-reversing, several elements are larger than uint8_t can handle.

>> +/* NOTE: The first element is always 16, unscaled. */
>> +static const uint16_t unscaled_quant_matrix[64] = {
> This can be uint8_t

It can. Will fix.

>> +static inline int decode_dc_le(GetBitContext *gb, int component)
>> +{
>> +    int code, diff;
>> +
>> +    if (component == 0 || component == 3) {
>> +        code = get_vlc2(gb, ff_dc_lum_vlc_le.table, DC_VLC_BITS, 2);
>> +    } else {
>> +        code = get_vlc2(gb, ff_dc_chroma_vlc_le.table, DC_VLC_BITS, 2);
>> +    }
>> +    if (code < 0) {
>> +        av_log(NULL, AV_LOG_ERROR, "invalid dc code at\n");
>> +        return 0xffff;
> Why this specific return value? I suppose decoding other blocks still continue?

I don't know; this function was cloned from decode_dc(), so I wanted to be
consistent.

>> +        /* Exactly the same as MPEG-2, except little-endian. */
>> +        reverse_code(ff_mpeg12_vlc_dc_lum_code,
>> +                     ff_mpeg12_vlc_dc_lum_bits,
>> +                     ff_mpeg12_vlc_dc_lum_code_reversed,
>> +                     12);
>> +        INIT_LE_VLC_STATIC(&ff_dc_lum_vlc_le, DC_VLC_BITS, 12,
>> +                           ff_mpeg12_vlc_dc_lum_bits, 1, 1,
>> +                           ff_mpeg12_vlc_dc_lum_code_reversed, 2, 2, 512);
>> +        reverse_code(ff_mpeg12_vlc_dc_chroma_code,
>> +                     ff_mpeg12_vlc_dc_chroma_bits,
>> +                     ff_mpeg12_vlc_dc_chroma_code_reversed,
>> +                     12);
> What about "storing" reverse codes in source code, so this step is not required?

Yes, I've considered it, but ultimately decided not to. Mainly, I consider it
an artifact of the current way INIT_VLC_LE works, and I'm not convinced it
will stay that way forever. Generally having multiple copies lying around
makes for duplication, and also for harder-to-understand code (it's better if
the code is uniformly stored/written in the source, independently of the
bit-packing convention in a lower layer).

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


More information about the ffmpeg-devel mailing list