[FFmpeg-devel] [PATCH 2/2] lavf/mp3enc: write encoder delay/padding upon closing

[Andreas Cadhalpun]

On 05.10.2016 21:37, Jon Toohill wrote:
> ---
>  libavformat/mp3enc.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c
> index de63401..ddf4b93 100644
> --- a/libavformat/mp3enc.c
> +++ b/libavformat/mp3enc.c
[...]
> @@ -345,10 +342,24 @@ static int mp3_write_audio_packet(AVFormatContext *s, AVPacket *pkt)
>  #endif
>  
>          if (mp3->xing_offset) {
> +            uint8_t *side_data = NULL;
> +            int side_data_size = 0;
> +
>              mp3_xing_add_frame(mp3, pkt);
>              mp3->audio_size += pkt->size;
>              mp3->audio_crc   = av_crc(av_crc_get_table(AV_CRC_16_ANSI_LE),
>                                        mp3->audio_crc, pkt->data, pkt->size);
> +
> +            side_data = av_packet_get_side_data(pkt,
> +                                                AV_PKT_DATA_SKIP_SAMPLES,
> +                                                &side_data_size);
> +            if (side_data && side_data_size >= 10) {
> +                mp3->padding = AV_RL32(side_data + 4) + 528 + 1;

I think this should also use FFMAX(..., 0), as the side_data could theoretically contain
a negative value.

> +                if (!mp3->delay)
> +                    mp3->delay =  FFMAX(AV_RL32(side_data) - 528 - 1, 0);
> +            } else {
> +                mp3->padding = 0;
> +            }
>          }
>      }
>  
> @@ -381,7 +392,7 @@ static void mp3_update_xing(AVFormatContext *s)
>      AVReplayGain *rg;
>      uint16_t tag_crc;
>      uint8_t *toc;
> -    int i, rg_size;
> +    int i, rg_size, delay;

This variable is not used.

>      /* replace "Xing" identification string with "Info" for CBR files. */
>      if (!mp3->has_variable_bitrate)
> @@ -422,6 +433,15 @@ static void mp3_update_xing(AVFormatContext *s)
>          }
>      }
>  
> +    /* write encoder delay/padding */
> +    if (mp3->delay >= 1 << 12) {
> +        av_log(s, AV_LOG_WARNING, "Too many samples of initial padding.\n");

I think this should cap mp3->delay to prevent setting unrelated bits in the header:
           mp3->delay = (1 << 12) - 1;

> +    }
> +    if (mp3->padding >= 1 << 12) {
> +        av_log(s, AV_LOG_WARNING, "Too many samples of trailing padding.\n");

Same as above.

> +    }
> +    AV_WB24(mp3->xing_frame + mp3->xing_offset + 141, (mp3->delay << 12) + mp3->padding);
> +
>      AV_WB32(mp3->xing_frame + mp3->xing_offset + XING_SIZE - 8, mp3->audio_size);
>      AV_WB16(mp3->xing_frame + mp3->xing_offset + XING_SIZE - 4, mp3->audio_crc);
>  
> 

Otherwise this patch looks good (and is definitely better than my previous attempt
of fixing this [1]).

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185657.html