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

[Michael Niedermayer]

On Wed, Dec 19, 2018 at 10:35:25AM +0100, Jerome Martinez wrote:
> 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?

The encoder side factors 2 different termination procedures. If you need
version 1 there you need version 1 you cannot use version 0 in its place.
The decoder has to deal with buggy input and the kind of termination needed
depends on where the termination is done it is not 1:1 bound to the bitstream
version.
The encoder OTOH does not produce the buggy variants so it does not have
anything symmetric for that.

There are also 3 places where termination happens, each is different
even if one disregards the old bug.
One place also needs further investigation to understand the implications
for the bitstream compatibility in case its changed.

So while i can factor the decoder side in a way similar to the encoder
this will still not make the code look more similar.

So i suggest to apply this patchset as it is.
I do have some patches locally that mess with the decoder side of the related
code but the code does not become simpler even thhough it does get checked a
bit more fully. So this is not really what you asked for IIUC

thx


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181223/d11dab10/attachment.sig>