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

[Tyler Jones]

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
-------------- 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/20170823/73eb4227/attachment.sig>