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

[Michael Niedermayer]

On Sun, Dec 23, 2018 at 04:05:12PM +0100, Michael Niedermayer wrote:
> 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

Will apply this patchset soon unless there are objections

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20181228/99857d77/attachment.sig>