[FFmpeg-devel] [PATCH 2/2] avcodec/vorbisenc: Apply dynamic frame lengths

[Tyler Jones]

On Wed, Jul 26, 2017 at 01:01:00AM +0100, Rostislav Pehlivanov wrote:
> On 12 July 2017 at 23:18, Tyler Jones <tdjones879 at gmail.com> wrote:
> 
> >
> > diff --git a/libavcodec/vorbis.c b/libavcodec/vorbis.c
> > index 399020e..8befab8 100644
> > --- a/libavcodec/vorbis.c
> > +++ b/libavcodec/vorbis.c
> > @@ -59,7 +59,7 @@ int ff_vorbis_len2vlc(uint8_t *bits, uint32_t *codes,
> > unsigned num)
> >      unsigned i, j, p, code;
> >
> >      for (p = 0; (bits[p] == 0) && (p < num); ++p)
> > -        ;
> > +        codes[p] = 0;
> >      if (p == num)
> >          return 0;
> >
> > @@ -78,9 +78,11 @@ int ff_vorbis_len2vlc(uint8_t *bits, uint32_t *codes,
> > unsigned num)
> >
> >      for (; p < num; ++p) {
> >          if (bits[p] > 32)
> > -             return AVERROR_INVALIDDATA;
> > -        if (bits[p] == 0)
> > -             continue;
> > +            return AVERROR_INVALIDDATA;
> > +        if (bits[p] == 0) {
> > +            codes[p] = 0;
> > +            continue;
> > +        }
> >
> 
> I prefer the if (!bits[p]) way of checking for 0. Most of the codebase does
> so too.

Agreed. I'll change this.

> > diff --git a/libavcodec/vorbis_enc_data.h b/libavcodec/vorbis_enc_data.h
> > index a51aaec..eca43df 100644
> > --- a/libavcodec/vorbis_enc_data.h
> > +++ b/libavcodec/vorbis_enc_data.h
> > @@ -23,15 +23,78 @@
> >
> >  #include <stdint.h>
> >
> > <TABLE CHANGES>
> >
> 
> Could you move the tables to vorbis_data.c and delete vorbis_enc_data.h?
> Would be neater.

My only hesitation is that these tables are only used in the encoder. I like
the additional clarity from the name, but I can change this.

> 
> Apart from those nits, patch would be fine for merging as well.
> 
> It improves quality in some ways however it introduces some clicking which
> I believe is due to the lack of stability.
> I think you can improve this by tweaking the constants in the previous
> patch and by reducing the fluctuations between
> transient and non-transient frames. Its better to have a whole series of
> transients rather than interrupting them with non-transients, and vice
> versa.

I'll see if I can introduce some sort of bias to discourage frequent switching.
I appreciate you taking a look and providing feedback.

Thanks as always,

Tyler Jones
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170725/cde6e9d8/attachment.sig>