Re: [FFmpeg-devel] [PATCH 2/6] avcodec/vorbisenc: Apply dynamic frame lengths
On Wed, Aug 23, 2017 at 10:31:58AM +0200, Tomas Härdin wrote: > On 2017-08-22 03:23, Tyler Jones wrote: > > +static int create_residues(vorbis_enc_context *venc) > > +{ > > +int res, ret; > > +vorbis_enc_residue *rc; > > + > > +venc->nresidues = 2; > > +venc->residues = av_malloc(sizeof(vorbis_enc_residue) * > > venc->nresidues); > > av_malloc_array()? Applies to most av_malloc() in there I can change it, but I don't feel that it helps readability in this specific case above. As for the others that happen to show up in the diffs, I did not want to make any unnecessary and unrelated functional changes. However, I'll gladly to switch these cases to `av_malloc_array()` in a separate commit if desired. > > -// single mapping > > -mc = &venc->mappings[0]; > > -mc->submaps = 1; > > -mc->mux = av_malloc(sizeof(int) * venc->channels); > > -if (!mc->mux) > > -return AVERROR(ENOMEM); > > -for (i = 0; i < venc->channels; i++) > > -mc->mux[i] = 0; > > -mc->floor = av_malloc(sizeof(int) * mc->submaps); > > -mc->residue = av_malloc(sizeof(int) * mc->submaps); > > -if (!mc->floor || !mc->residue) > > -return AVERROR(ENOMEM); > > -for (i = 0; i < mc->submaps; i++) { > > -mc->floor[i] = 0; > > -mc->residue[i] = 0; > > -} > > -mc->coupling_steps = venc->channels == 2 ? 1 : 0; > > -mc->magnitude = av_malloc(sizeof(int) * mc->coupling_steps); > > -mc->angle = av_malloc(sizeof(int) * mc->coupling_steps); > > -if (!mc->magnitude || !mc->angle) > > -return AVERROR(ENOMEM); > > -if (mc->coupling_steps) { > > -mc->magnitude[0] = 0; > > -mc->angle[0] = 1; > > +for (map = 0; map < venc->nmappings; map++) { > > +mc = &venc->mappings[map]; > > +mc->submaps = 1; > > +mc->mux = av_malloc(sizeof(int) * venc->channels); > > +if (!mc->mux) > > +return AVERROR(ENOMEM); > > +for (i = 0; i < venc->channels; i++) > > +mc->mux[i] = 0; > > +mc->floor = av_malloc(sizeof(int) * mc->submaps); > > +mc->residue = av_malloc(sizeof(int) * mc->submaps); > > +if (!mc->floor || !mc->residue) > > +return AVERROR(ENOMEM); > > +for (i = 0; i < mc->submaps; i++) { > > +mc->floor[i] = map; > > +mc->residue[i] = map; > > +} > > +mc->coupling_steps = venc->channels == 2 ? 1 : 0; > > +mc->magnitude = av_malloc(sizeof(int) * mc->coupling_steps); > > +mc->angle = av_malloc(sizeof(int) * mc->coupling_steps); > > +if (!mc->magnitude || !mc->angle) > > +return AVERROR(ENOMEM); > > +if (mc->coupling_steps) { > > +mc->magnitude[0] = 0; > > +mc->angle[0] = 1; > > +} > > } > > Maybe nitpicking, but it would be clearer what the changes are if you put > the indentation change in a separate commit No, you're right, and it's a good suggestion. I'll move the indentation to a separate commit when enough other changes have been provided to warrant a new version. > > -move_audio(venc, avctx->frame_size); > > +if (venc->transient < 0) { > > +move_audio(venc, avctx->frame_size); > > -for (ch = 0; ch < venc->channels; ch++) { > > -float *scratch = venc->scratch + 2 * ch * frame_size + frame_size; > > +for (ch = 0; ch < venc->channels; ch++) { > > +float *scratch = venc->scratch + 2 * ch * long_win + long_win; > > -if (!ff_psy_vorbis_block_frame(&venc->vpctx, scratch, ch, > > - frame_size, block_size)) > > -curr_win = 0; > > +if (!ff_psy_vorbis_block_frame(&venc->vpctx, scratch, ch, > > + long_win, short_win)) > > +next_win = 0; > > +} > > } > > Same here > > /Tomas > I felt that separating this small amount of lines would just clutter the git log history, but I'll move these along with the mapping indentations. Thanks for taking a look, Tyler Jones signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/6] avcodec/vorbisenc: Apply dynamic frame lengths
On 2017-08-22 03:23, Tyler Jones wrote: +static int create_residues(vorbis_enc_context *venc) +{ +int res, ret; +vorbis_enc_residue *rc; + +venc->nresidues = 2; +venc->residues = av_malloc(sizeof(vorbis_enc_residue) * venc->nresidues); av_malloc_array()? Applies to most av_malloc() in there -// single mapping -mc = &venc->mappings[0]; -mc->submaps = 1; -mc->mux = av_malloc(sizeof(int) * venc->channels); -if (!mc->mux) -return AVERROR(ENOMEM); -for (i = 0; i < venc->channels; i++) -mc->mux[i] = 0; -mc->floor = av_malloc(sizeof(int) * mc->submaps); -mc->residue = av_malloc(sizeof(int) * mc->submaps); -if (!mc->floor || !mc->residue) -return AVERROR(ENOMEM); -for (i = 0; i < mc->submaps; i++) { -mc->floor[i] = 0; -mc->residue[i] = 0; -} -mc->coupling_steps = venc->channels == 2 ? 1 : 0; -mc->magnitude = av_malloc(sizeof(int) * mc->coupling_steps); -mc->angle = av_malloc(sizeof(int) * mc->coupling_steps); -if (!mc->magnitude || !mc->angle) -return AVERROR(ENOMEM); -if (mc->coupling_steps) { -mc->magnitude[0] = 0; -mc->angle[0] = 1; +for (map = 0; map < venc->nmappings; map++) { +mc = &venc->mappings[map]; +mc->submaps = 1; +mc->mux = av_malloc(sizeof(int) * venc->channels); +if (!mc->mux) +return AVERROR(ENOMEM); +for (i = 0; i < venc->channels; i++) +mc->mux[i] = 0; +mc->floor = av_malloc(sizeof(int) * mc->submaps); +mc->residue = av_malloc(sizeof(int) * mc->submaps); +if (!mc->floor || !mc->residue) +return AVERROR(ENOMEM); +for (i = 0; i < mc->submaps; i++) { +mc->floor[i] = map; +mc->residue[i] = map; +} +mc->coupling_steps = venc->channels == 2 ? 1 : 0; +mc->magnitude = av_malloc(sizeof(int) * mc->coupling_steps); +mc->angle = av_malloc(sizeof(int) * mc->coupling_steps); +if (!mc->magnitude || !mc->angle) +return AVERROR(ENOMEM); +if (mc->coupling_steps) { +mc->magnitude[0] = 0; +mc->angle[0] = 1; +} } Maybe nitpicking, but it would be clearer what the changes are if you put the indentation change in a separate commit -move_audio(venc, avctx->frame_size); +if (venc->transient < 0) { +move_audio(venc, avctx->frame_size); -for (ch = 0; ch < venc->channels; ch++) { -float *scratch = venc->scratch + 2 * ch * frame_size + frame_size; +for (ch = 0; ch < venc->channels; ch++) { +float *scratch = venc->scratch + 2 * ch * long_win + long_win; -if (!ff_psy_vorbis_block_frame(&venc->vpctx, scratch, ch, - frame_size, block_size)) -curr_win = 0; +if (!ff_psy_vorbis_block_frame(&venc->vpctx, scratch, ch, + long_win, short_win)) +next_win = 0; +} } Same here /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/6] avcodec/vorbisenc: Apply dynamic frame lengths
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 --- V4: No changes V3: Switch 'bits[p] == 0' to '!bits[p]' in vlc gen V2: Fix double arithmetic in window scale libavcodec/vorbis.c | 10 +- libavcodec/vorbis_enc_data.h | 289 +++-- libavcodec/vorbisenc.c | 424 ++- tests/fate/vorbis.mak| 2 +- 4 files changed, 454 insertions(+), 271 deletions(-) diff --git a/libavcodec/vorbis.c b/libavcodec/vorbis.c index 399020eec5..d8c4b006e7 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]) { +codes[p] = 0; +continue; +} // 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 a51aaec978..eca43dfded 100644 --- a/libavcodec/vorbis_enc_data.h +++ b/libavcodec/vorbis_enc_data.h @@ -23,15 +23,78 @@ #include -static const uint8_t codebook0[] = { +static const uint8_t floor_128_c0[] = { +10, 7, 8, 13, 9, 6, 7, 11, 10, 8, 8, 12, 17, 17, 17, +17, 7, 5, 5, 9, 6, 4, 4, 8, 8, 5, 5, 8, 16, 14, +13, 16, 7, 5, 5, 7, 6, 3, 3, 5, 8, 5, 4, 7, 14, +12, 12, 15, 10, 7, 8, 9, 7, 5, 5, 6, 9, 6, 5, 5, +15, 12, 9, 10, +}; + +static const uint8_t floor_128_c1[] = { + 8, 13, 17, 17, 8, 11, 17, 17, 11, 13, 17, 17, 17, 17, 17, +17, 6, 10, 16, 17, 6, 10, 15, 17, 8, 10, 16, 17, 17, 17, +17, 17, 9, 13, 15, 17, 8, 11, 17, 17, 10, 12, 17, 17, 17, +17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, +17, 17, 17, 17, 6, 11, 15, 17, 7, 10, 15, 17, 8, 10, 17, +17, 17, 15, 17, 17, 4, 8, 13, 17, 4, 7, 13, 17, 6, 8, +15, 17, 16, 15, 17, 17, 6, 11, 15, 17, 6, 9, 13, 17, 8, +10, 17, 17, 15, 17, 17, 17, 16, 17, 17, 17, 12, 14, 15, 17, +13, 14, 15, 17, 17, 17, 17, 17, 5, 10, 14, 17, 5, 9, 14, +17, 7, 9, 15, 17, 15, 15, 17, 17, 3, 7, 12, 17, 3, 6, +11, 17, 5, 7, 13, 17, 12, 12, 17, 17, 5, 9, 14, 17, 3, + 7, 11, 17, 5, 8, 13, 17, 13, 11, 16, 17, 12, 17, 17, 17, + 9, 14, 15, 17, 10, 11, 14, 17, 16, 14, 17, 17, 8, 12, 17, +17, 8, 12, 17, 17, 10, 12, 17, 17, 17, 17, 17, 17, 5, 10, +17, 17, 5, 9, 15, 17, 7, 9, 17, 17, 13, 13, 17, 17, 7, +11, 17, 17, 6, 10, 15, 17, 7, 9, 15, 17, 12, 11, 17, 17, +12, 15, 17, 17, 11, 14, 17, 17, 11, 10, 15, 17, 17, 16, 17, +17, +}; + +static const uint8_t floor_128_0sub1[] = { + 0, 3, 3, 3, 3, 3, 3, 3, 3, +}; + +static const uint8_t floor_128_0sub2[] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 3, 3, 4, 4, 4, + 4, 5, 4, 5, 4, 5, 4, 6, 4, 6, +}; + +static const uint8_t floor_128_0sub3[] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 5, 3, 5, 3, + 5, 4, 5, 4, 5, 5, 5, 5, 6, 5, 6, 5, 6, 5, 6, + 5, 6, 5, 7, 8, 9, 11, 13, 13, 13, 13, 13, 13, 13, 13, +13, 13, 13, 13, +}; + +static const uint8_t floor_128_1sub1[] = { + 0, 3, 3, 2, 3, 3, 4, 3, 4, +}; + +static const uint8_t floor_128_1sub2[] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 4, 3, 6, 3, 6, + 3, 6, 3, 7, 3, 8, 4, 9, 4, 9, +}; + +static const uint8_t floor_128_1sub3[] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 7, 2, 7, 3, + 8, 4, 9, 5, 9, 8, 10, 11, 11, 12, 14, 14, 14, 14, 14, +14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, +13, 13, 13, 13, +}; + +static const uint8_t floor_1024_c1[] = { 2, 10, 8, 14, 7, 12, 11, 14, 1, 5, 3, 7, 4, 9, 7, 13, }; -static const uint8_t codebook1[] = { +static const uint8_t floor_1024_c2[] = { 1, 4, 2, 6, 3, 7, 5, 7, }; -static const uint8_t codebook2[] = { +static const uint8_t floor_1024_c3[] = { 1, 5, 7, 21, 5, 8, 9, 21, 10, 9, 12, 20, 20, 16, 20, 20, 4, 8,