| Message ID | 20181219014020.16299-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 1be9a28f8e8cd2553f978670901174a8808b9a1a |
| Headers | show |
On 19/12/2018 02:40, Michael Niedermayer wrote: > Signed-off-by: Michael Niedermayer <michael@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?
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@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 [...]
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@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 [...]
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; init_put_bits(&fs->pb, fs->c.bytestream_start + fs->ac_byte_count, fs->c.bytestream_end - fs->c.bytestream_start - fs->ac_byte_count); @@ -1232,9 +1230,7 @@ FF_ENABLE_DEPRECATION_WARNINGS int bytes; if (fs->ac != AC_GOLOMB_RICE) { - uint8_t state = 129; - put_rac(&fs->c, &state, 0); - bytes = ff_rac_terminate(&fs->c); + bytes = ff_rac_terminate(&fs->c, 1); } else { flush_put_bits(&fs->pb); // FIXME: nicer padding bytes = fs->ac_byte_count + (put_bits_count(&fs->pb) + 7) / 8; diff --git a/libavcodec/rangecoder.c b/libavcodec/rangecoder.c index 0d53bef076..fa7d5526d1 100644 --- a/libavcodec/rangecoder.c +++ b/libavcodec/rangecoder.c @@ -106,8 +106,10 @@ void ff_build_rac_states(RangeCoder *c, int factor, int max_p) } /* Return the number of bytes written. */ -int ff_rac_terminate(RangeCoder *c) +int ff_rac_terminate(RangeCoder *c, int version) { + if (version == 1) + put_rac(c, (uint8_t[]) { 129 }, 0); c->range = 0xFF; c->low += 0xFF; renorm_encoder(c); diff --git a/libavcodec/rangecoder.h b/libavcodec/rangecoder.h index 44af88b8f5..e5c7a3cc71 100644 --- a/libavcodec/rangecoder.h +++ b/libavcodec/rangecoder.h @@ -48,7 +48,7 @@ typedef struct RangeCoder { void ff_init_range_encoder(RangeCoder *c, uint8_t *buf, int buf_size); void ff_init_range_decoder(RangeCoder *c, const uint8_t *buf, int buf_size); -int ff_rac_terminate(RangeCoder *c); +int ff_rac_terminate(RangeCoder *c, int version); void ff_build_rac_states(RangeCoder *c, int factor, int max_p); static inline void renorm_encoder(RangeCoder *c) diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c index 61a658fa44..df1729a083 100644 --- a/libavcodec/snowenc.c +++ b/libavcodec/snowenc.c @@ -1899,7 +1899,7 @@ FF_DISABLE_DEPRECATION_WARNINGS FF_ENABLE_DEPRECATION_WARNINGS #endif - pkt->size = ff_rac_terminate(c); + pkt->size = ff_rac_terminate(c, 0); if (s->current_picture->key_frame) pkt->flags |= AV_PKT_FLAG_KEY; *got_packet = 1; diff --git a/libavcodec/sonic.c b/libavcodec/sonic.c index 2e3ca79fdd..34d2952e69 100644 --- a/libavcodec/sonic.c +++ b/libavcodec/sonic.c @@ -842,7 +842,7 @@ static int sonic_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, // av_log(avctx, AV_LOG_DEBUG, "used bytes: %d\n", (put_bits_count(&pb)+7)/8); - avpkt->size = ff_rac_terminate(&c); + avpkt->size = ff_rac_terminate(&c, 0); *got_packet_ptr = 1; return 0; diff --git a/libavcodec/tests/rangecoder.c b/libavcodec/tests/rangecoder.c index 2da5c0ce33..76a13e7091 100644 --- a/libavcodec/tests/rangecoder.c +++ b/libavcodec/tests/rangecoder.c @@ -48,7 +48,7 @@ int main(void) for (i = 0; i < SIZE; i++) put_rac(&c, state, r[i] & 1); - ff_rac_terminate(&c); + ff_rac_terminate(&c, 0); ff_init_range_decoder(&c, b, SIZE);
Signed-off-by: Michael Niedermayer <michael@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(-)