| Message ID | 1475696257-12390-3-git-send-email-jtoohill@google.com |
|---|---|
| State | Superseded |
| Headers | show |
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
diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c index de63401..ddf4b93 100644 --- a/libavformat/mp3enc.c +++ b/libavformat/mp3enc.c @@ -111,6 +111,8 @@ typedef struct MP3Context { uint64_t bag[XING_NUM_BAGS]; int initial_bitrate; int has_variable_bitrate; + int delay; + int padding; /* index of the audio stream */ int audio_stream_idx; @@ -247,12 +249,7 @@ static int mp3_write_xing(AVFormatContext *s) ffio_fill(dyn_ctx, 0, 8); // empty replaygain fields avio_w8(dyn_ctx, 0); // unknown encoding flags avio_w8(dyn_ctx, 0); // unknown abr/minimal bitrate - - // encoder delay - if (par->initial_padding - 528 - 1 >= 1 << 12) { - av_log(s, AV_LOG_WARNING, "Too many samples of initial padding.\n"); - } - avio_wb24(dyn_ctx, FFMAX(par->initial_padding - 528 - 1, 0)<<12); + avio_wb24(dyn_ctx, 0); // empty encoder delay/padding avio_w8(dyn_ctx, 0); // misc avio_w8(dyn_ctx, 0); // mp3gain @@ -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; + 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; /* 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"); + } + if (mp3->padding >= 1 << 12) { + av_log(s, AV_LOG_WARNING, "Too many samples of trailing padding.\n"); + } + 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);