[FFmpeg-devel] [PATCH 1/3] avcodec/rangecoder: factorize termination version code

[Jerome Martinez]

On 19/12/2018 02:40, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>   libavcodec/ffv1enc.c          | 10 +++-------
>   libavcodec/rangecoder.c       |  4 +++-
>   libavcodec/rangecoder.h       |  2 +-
>   libavcodec/snowenc.c          |  2 +-
>   libavcodec/sonic.c            |  2 +-
>   libavcodec/tests/rangecoder.c |  2 +-
>   6 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c
> index f5eb0feb4e..796d81f7c6 100644
> --- a/libavcodec/ffv1enc.c
> +++ b/libavcodec/ffv1enc.c
> @@ -449,7 +449,7 @@ static int write_extradata(FFV1Context *f)
>           put_symbol(c, state, f->intra = (f->avctx->gop_size < 2), 0);
>       }
>   
> -    f->avctx->extradata_size = ff_rac_terminate(c);
> +    f->avctx->extradata_size = ff_rac_terminate(c, 0);
>       v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0, f->avctx->extradata, f->avctx->extradata_size);
>       AV_WL32(f->avctx->extradata + f->avctx->extradata_size, v);
>       f->avctx->extradata_size += 4;
> @@ -1065,9 +1065,7 @@ retry:
>           encode_slice_header(f, fs);
>       }
>       if (fs->ac == AC_GOLOMB_RICE) {
> -        if (f->version > 2)
> -            put_rac(&fs->c, (uint8_t[]) { 129 }, 0);
> -        fs->ac_byte_count = f->version > 2 || (!x && !y) ? ff_rac_terminate(&fs->c) : 0;
> +        fs->ac_byte_count = f->version > 2 || (!x && !y) ? ff_rac_terminate(&fs->c, f->version > 2) : 0;

Moving the "129" stuff from FFV1 encoder code to rangecoder encoding 
part is good for factorization, but there is no more mirroring of FFV1 
encoding and FFV1 decoding in that case ("129" stuff is still present in 
FFV1 decoder code, not rangecoder decoding part), IMO this makes the 
FFV1 code more difficult to understand.

Isn't it possible to have the same kind of modification for the decoding 
part?