Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-06-12 Thread Michael Niedermayer 
On Wed, Jun 08, 2016 at 10:46:49AM -0700, Jon Toohill wrote:
> Michael et al., is this good to merge as-is? I just tested and a round trip
> with ffmpeg from wav -> mp3 -> wav retains the correct number of samples.

There seems to be a inconsistency 

try the patch below with your patch, and write a mp3 file with ffmpeg
and the read it the initial_padding is not what was written

or try this:
/ffmpeg -i test.mp3 -codec copy test2.mp3
read padding 576
written padding 576

./ffmpeg -i test2.mp3 -codec copy test3.mp3
read padding 47
written padding 47


diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 345fa88..b063ad9 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -240,6 +240,7 @@ 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;
+av_log(0,0, "read padding %d\n", st->codecpar->initial_padding );
 st->codecpar->trailing_padding = mp3->end_pad;
 st->start_skip_samples = mp3->start_pad + 528 + 1;
 if (mp3->frames) {
diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c
index de63401..a971136 100644
--- a/libavformat/mp3enc.c
+++ b/libavformat/mp3enc.c
@@ -253,7 +253,7 @@ static int mp3_write_xing(AVFormatContext *s)
 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);
-
+av_log(0,0, "written padding %d\n", par->initial_padding );
 avio_w8(dyn_ctx,   0); // misc
 avio_w8(dyn_ctx,   0); // mp3gain
 avio_wb16(dyn_ctx, 0); // preset

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-06-08 Thread Jon Toohill 
Michael et al., is this good to merge as-is? I just tested and a round trip
with ffmpeg from wav -> mp3 -> wav retains the correct number of samples.


Jon Toohill |  Google Play Music |  jtooh...@google.com |  (650) 215-0770

On Wed, Jun 1, 2016 at 5:58 PM, Jon Toohill  wrote:

> Based on my understanding of [1], these values in the Info tag specify
> only the encoder delay/padding, which matches the documentation for these
> fields. It looks like other formats are using the fields that way as well.
>
> I think the extra 528 + 1 samples are the decoder delay [2]. It looks like
> libmp3lame adds the 528 + 1 only to have mp3dec subtract it, so I'm not
> sure why that's done. IIUC start_skip_samples is the mechanism that
> actually accounts for the extra delay when decoding.
>
> [1]: http://gabriel.mp3-tech.org/mp3infotag.html#delays
> [2]: http://lame.sourceforge.net/tech-FAQ.txt
>
>
>
> Jon Toohill |  Google Play Music |  jtooh...@google.com |  (650) 215-0770
>
> On Thu, May 26, 2016 at 7:51 PM, Michael Niedermayer <
> mich...@niedermayer.cc> wrote:
>
>> On Wed, May 25, 2016 at 09:56:59AM -0700, Jon Toohill wrote:
>> > ---
>> >  libavformat/mp3dec.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
>> > index 3725d67..192f5ef 100644
>> > --- a/libavformat/mp3dec.c
>> > +++ b/libavformat/mp3dec.c
>> > @@ -234,6 +234,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;
>> > +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;
>>
>> is the 528 + 1 difference intended to
>> start_skip_samples/first_discard_sample
>> ?
>> mp3enc stores par->initial_padding - 528 - 1
>>
>> [...]
>>
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> If you drop bombs on a foreign country and kill a hundred thousand
>> innocent people, expect your government to call the consequence
>> "unprovoked inhuman terrorist attacks" and use it to justify dropping
>> more bombs and killing more people. The technology changed, the idea is
>> old.
>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-06-01 Thread Jon Toohill 
Based on my understanding of [1], these values in the Info tag specify only
the encoder delay/padding, which matches the documentation for these
fields. It looks like other formats are using the fields that way as well.

I think the extra 528 + 1 samples are the decoder delay [2]. It looks like
libmp3lame adds the 528 + 1 only to have mp3dec subtract it, so I'm not
sure why that's done. IIUC start_skip_samples is the mechanism that
actually accounts for the extra delay when decoding.

[1]: http://gabriel.mp3-tech.org/mp3infotag.html#delays
[2]: http://lame.sourceforge.net/tech-FAQ.txt



Jon Toohill |  Google Play Music |  jtooh...@google.com |  (650) 215-0770

On Thu, May 26, 2016 at 7:51 PM, Michael Niedermayer  wrote:

> On Wed, May 25, 2016 at 09:56:59AM -0700, Jon Toohill wrote:
> > ---
> >  libavformat/mp3dec.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 3725d67..192f5ef 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -234,6 +234,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;
> > +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;
>
> is the 528 + 1 difference intended to
> start_skip_samples/first_discard_sample
> ?
> mp3enc stores par->initial_padding - 528 - 1
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you drop bombs on a foreign country and kill a hundred thousand
> innocent people, expect your government to call the consequence
> "unprovoked inhuman terrorist attacks" and use it to justify dropping
> more bombs and killing more people. The technology changed, the idea is
> old.
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-05-26 Thread Michael Niedermayer 
On Wed, May 25, 2016 at 09:56:59AM -0700, Jon Toohill wrote:
> ---
>  libavformat/mp3dec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 3725d67..192f5ef 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -234,6 +234,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;
> +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;

is the 528 + 1 difference intended to start_skip_samples/first_discard_sample
?
mp3enc stores par->initial_padding - 528 - 1

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-05-26 Thread James Almer 
On 5/26/2016 11:05 PM, Michael Niedermayer wrote:
> On Wed, May 25, 2016 at 02:18:38PM -0400, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Wed, May 25, 2016 at 1:24 PM, James Almer  wrote:
>>
>>> On 5/25/2016 1:56 PM, Jon Toohill wrote:
 ---
  libavformat/mp3dec.c | 2 ++
  1 file changed, 2 insertions(+)

 diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
 index 3725d67..192f5ef 100644
 --- a/libavformat/mp3dec.c
 +++ b/libavformat/mp3dec.c
 @@ -234,6 +234,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;
 +st->codecpar->trailing_padding = mp3->end_pad;
>>>
>>> Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data
>>> to
>>> discard samples from the last packet/frame. See matroska and ogg demuxers,
>>> currently used for Opus only.
>>>
>>> The trailing_padding AVCodecParameters element was added after the above
>>> was
>>> designed. To be honest i can't say if we should replace one with the other
>>> or find a way to keep both, and seeing how AVCodecParameters hasn't made it
>>> to a release yet, we're still on time to choose.
>>
>>
>> I agree having 1 is better than having 2. I can't technically comment on
>> which one is better/easier/*.
> 
> mp3 supports AV_PKT_DATA_SKIP_SAMPLES since FFmpeg 2.5
> ogg opus sets codecpar->initial_padding like this patch would
> so does dtshddec and matroskadec
> 
> AV_PKT_DATA_SKIP_SAMPLES is not a substitute for setting
> trailing_padding because with AV_PKT_DATA_SKIP_SAMPLES the
> trailing_padding only becomes available at the end of the stream
> 
> also theres nothing wrong with AV_PKT_DATA_SKIP_SAMPLES, its local
> information for the current packet and there may indeed be cases
> like with concatenated streams where there are packets in the middle
> with discarding. So the 2 AVCodecParameters fields are not a substitute
> for AV_PKT_DATA_SKIP_SAMPLES
> 

Fine with me, then. I assumed both both were pretty much made for the same
purpose and to cover the same use cases.

> also for encoding AV_PKT_DATA_SKIP_SAMPLES does not work with some
> formats the trailing_padding is needed to be known when writing the
> header

The one format i know should be using AV_PKT_DATA_SKIP_SAMPLES the same way
as ogg and matroska is mpegts. Probably also mp4.
At least for Opus the mpegts muxer reads the value from the last packet the
encoder or demuxer sends its way and writes it to the output file, but the
mpegts demuxer is apparently none the wiser because the opus parser just
discards this information.

> 
> the patch does look like a reasonable step to me but i might be
> missing something

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-05-26 Thread Michael Niedermayer 
On Wed, May 25, 2016 at 02:18:38PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, May 25, 2016 at 1:24 PM, James Almer  wrote:
> 
> > On 5/25/2016 1:56 PM, Jon Toohill wrote:
> > > ---
> > >  libavformat/mp3dec.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > > index 3725d67..192f5ef 100644
> > > --- a/libavformat/mp3dec.c
> > > +++ b/libavformat/mp3dec.c
> > > @@ -234,6 +234,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;
> > > +st->codecpar->trailing_padding = mp3->end_pad;
> >
> > Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data
> > to
> > discard samples from the last packet/frame. See matroska and ogg demuxers,
> > currently used for Opus only.
> >
> > The trailing_padding AVCodecParameters element was added after the above
> > was
> > designed. To be honest i can't say if we should replace one with the other
> > or find a way to keep both, and seeing how AVCodecParameters hasn't made it
> > to a release yet, we're still on time to choose.
> 
> 
> I agree having 1 is better than having 2. I can't technically comment on
> which one is better/easier/*.

mp3 supports AV_PKT_DATA_SKIP_SAMPLES since FFmpeg 2.5
ogg opus sets codecpar->initial_padding like this patch would
so does dtshddec and matroskadec

AV_PKT_DATA_SKIP_SAMPLES is not a substitute for setting
trailing_padding because with AV_PKT_DATA_SKIP_SAMPLES the
trailing_padding only becomes available at the end of the stream

also theres nothing wrong with AV_PKT_DATA_SKIP_SAMPLES, its local
information for the current packet and there may indeed be cases
like with concatenated streams where there are packets in the middle
with discarding. So the 2 AVCodecParameters fields are not a substitute
for AV_PKT_DATA_SKIP_SAMPLES

also for encoding AV_PKT_DATA_SKIP_SAMPLES does not work with some
formats the trailing_padding is needed to be known when writing the
header

the patch does look like a reasonable step to me but i might be
missing something

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-05-25 Thread Ronald S. Bultje 
Hi,

On Wed, May 25, 2016 at 1:24 PM, James Almer  wrote:

> On 5/25/2016 1:56 PM, Jon Toohill wrote:
> > ---
> >  libavformat/mp3dec.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 3725d67..192f5ef 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -234,6 +234,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;
> > +st->codecpar->trailing_padding = mp3->end_pad;
>
> Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data
> to
> discard samples from the last packet/frame. See matroska and ogg demuxers,
> currently used for Opus only.
>
> The trailing_padding AVCodecParameters element was added after the above
> was
> designed. To be honest i can't say if we should replace one with the other
> or find a way to keep both, and seeing how AVCodecParameters hasn't made it
> to a release yet, we're still on time to choose.


I agree having 1 is better than having 2. I can't technically comment on
which one is better/easier/*.

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-05-25 Thread James Almer 
On 5/25/2016 1:56 PM, Jon Toohill wrote:
> ---
>  libavformat/mp3dec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 3725d67..192f5ef 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -234,6 +234,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;
> +st->codecpar->trailing_padding = mp3->end_pad;

Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data to
discard samples from the last packet/frame. See matroska and ogg demuxers,
currently used for Opus only.

The trailing_padding AVCodecParameters element was added after the above was
designed. To be honest i can't say if we should replace one with the other
or find a way to keep both, and seeing how AVCodecParameters hasn't made it
to a release yet, we're still on time to choose.

>  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;
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-05-25 Thread Jon Toohill 
---
 libavformat/mp3dec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 3725d67..192f5ef 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -234,6 +234,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;
+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;
-- 
2.8.0.rc3.226.g39d4020

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

2016-05-24 Thread Jon Toohill 
From: Jon Toohill 

---
 libavformat/mp3dec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 3725d67..192f5ef 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -234,6 +234,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;
+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;
-- 
2.8.0.rc3.226.g39d4020

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel