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

[Rostislav Pehlivanov]

On 12 July 2017 at 23:18, Tyler Jones <tdjones879 at gmail.com> wrote:

> Additional codebooks are added for shorter 128-sample frames. Changes in
> codeword generation are made to handle valid values of 0 that prepend some
> codebooks, otherwise books are classified incorrectly and cause unreadable
> streams.
>
> A second residue, floor, and mapping is created for short window lengths
> so that values are partitioned correctly for transient frames.
>
> Signed-off-by: Tyler Jones <tdjones879 at gmail.com>
> ---
>  libavcodec/vorbis.c          |  10 +-
>  libavcodec/vorbis_enc_data.h | 289 +++++++++++++++++++----------
>  libavcodec/vorbisenc.c       | 422 ++++++++++++++++++++++++++----
> -------------
>  tests/fate/vorbis.mak        |   2 +-
>  4 files changed, 453 insertions(+), 270 deletions(-)
>
> 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.


>          // find corresponding exit(node which the tree can grow further
> from)
>          for (i = bits[p]; i > 0; --i)
>              if (exit_at_level[i])
> 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.

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.