Message ID | 1474910019-13882-5-git-send-email-jtoohill@google.com |
---|---|
State | Superseded |
Headers | show |
On Mon, 26 Sep 2016 10:13:39 -0700 Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote: > --- > libavformat/mp3dec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > index 56c7f8c..9cc85a3 100644 > --- a/libavformat/mp3dec.c > +++ b/libavformat/mp3dec.c > @@ -239,6 +239,8 @@ static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st, > > mp3->start_pad = v>>12; > mp3-> end_pad = v&4095; > + st->codecpar->initial_padding = mp3->start_pad + 528 + 1; > + st->codecpar->trailing_padding = mp3->end_pad; > st->start_skip_samples = mp3->start_pad + 528 + 1; > if (mp3->frames) { > st->first_discard_sample = -mp3->end_pad + 528 + 1 + mp3->frames * (int64_t)spf; I'm somewhat suspicious about this, because mp3dec.c uses AV_PKT_DATA_SKIP_SAMPLES to communicate delay/padding (libavformat/utils.c turns the start_skip_samples field into side data). So I'm not quite convinced is this mess of FFmpeg and Libav API mixture is healthy. Opinions welcome.
A similar concern was raised in a previous related patch: http://ffmpeg.org/pipermail/ffmpeg-devel/2016-May/194690.html I think the resolution at the time was to go ahead with using both, since both are used widely and serve slightly different purposes, although I do agree that it's confusing. I think I could use AV_PKT_DATA_SKIP_SAMPLES here, and change mp3enc to use that to fill out the Info tag. But that only seems worthwhile to me if there is a general consensus to move to just AV_PKT_DATA_SKIP_SAMPLES (and deprecate/remove initial_padding and trailing_padding). If there's a concrete case where knowing trailing_padding at the start of a stream is necessary, then that's not possible, but I'm pretty new to this and don't know of one. Thoughts? Jon Toohill | Google Play Music | jtoohill@google.com | (650) 215-0770 On Mon, Sep 26, 2016 at 11:30 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Mon, 26 Sep 2016 10:13:39 -0700 > Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote: > > > --- > > libavformat/mp3dec.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > index 56c7f8c..9cc85a3 100644 > > --- a/libavformat/mp3dec.c > > +++ b/libavformat/mp3dec.c > > @@ -239,6 +239,8 @@ static void mp3_parse_info_tag(AVFormatContext *s, > AVStream *st, > > > > mp3->start_pad = v>>12; > > mp3-> end_pad = v&4095; > > + st->codecpar->initial_padding = mp3->start_pad + 528 + 1; > > + st->codecpar->trailing_padding = mp3->end_pad; > > st->start_skip_samples = mp3->start_pad + 528 + 1; > > if (mp3->frames) { > > st->first_discard_sample = -mp3->end_pad + 528 + 1 + > mp3->frames * (int64_t)spf; > > I'm somewhat suspicious about this, because mp3dec.c uses > AV_PKT_DATA_SKIP_SAMPLES to communicate delay/padding > (libavformat/utils.c turns the start_skip_samples field into side > data). So I'm not quite convinced is this mess of FFmpeg and Libav API > mixture is healthy. Opinions welcome. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, 26 Sep 2016 16:04:26 -0700 Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote: > A similar concern was raised in a previous related patch: > http://ffmpeg.org/pipermail/ffmpeg-devel/2016-May/194690.html > I think the resolution at the time was to go ahead with using both, since > both are used widely and serve slightly different purposes, although I do > agree that it's confusing. > > I think I could use AV_PKT_DATA_SKIP_SAMPLES here, and change mp3enc to use > that to fill out the Info tag. But that only seems worthwhile to me if > there is a general consensus to move to just AV_PKT_DATA_SKIP_SAMPLES (and > deprecate/remove initial_padding and trailing_padding). If there's a > concrete case where knowing trailing_padding at the start of a stream is > necessary, then that's not possible, but I'm pretty new to this and don't > know of one. Thoughts? AV_PKT_DATA_SKIP_SAMPLES is FFmpeg's own "recent" solution for handling gapless, the codecpar padding fields are Libav's newer solution for the same problem. Libav doesn't have AV_PKT_DATA_SKIP_SAMPLES. There's also AVCodecContext.delay, which is an older solution for the initial padding. We're entering a real mess here. So we should make a conscious decision about what mechanism should be used in the future, rather than making an ad-hoc decision for a single patch. (And then probably changing our opinion later, all contributing to furthering the mess.) So we need to decide a few things: - which mechanism should be used? - should the older mechanism(s) be deprecated? - do they conflict? how should compatibility be kept? - should decoding (i.e. in AVCodecContext) use the newer mechanism to change output data, like with AV_PKT_DATA_SKIP_SAMPLES? - should this patch be held back until a decision was made? (Unless I'm missing something, which is possible.) And of course we need to know how to make a decision, since we apparently don't have a process for this in this project.
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..9cc85a3 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -239,6 +239,8 @@ static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st, mp3->start_pad = v>>12; mp3-> end_pad = v&4095; + st->codecpar->initial_padding = mp3->start_pad + 528 + 1; + st->codecpar->trailing_padding = mp3->end_pad; st->start_skip_samples = mp3->start_pad + 528 + 1; if (mp3->frames) { st->first_discard_sample = -mp3->end_pad + 528 + 1 + mp3->frames * (int64_t)spf;