| Message ID | CANpj82Jb6HpNz7ARwFr8YSzndezzU22XxscWawx+suQSXRpgaQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 25, 2017 at 01:25:23PM +0200, Jean-Yves Avenard wrote: > From 9baa7166fa96ed6beac9146c7e3b4dcf425a67d0 Mon Sep 17 00:00:00 2001 > From: Jean-Yves Avenard <jyavenard@mozilla.com> > Date: Fri, 25 Aug 2017 13:11:28 +0200 > Subject: [PATCH] Properly store sampling rate for FLAC in mp4 > > Fixes ticket #6609 > > Signed-off-by: Jean-Yves Avenard <jyavenard@mozilla.com> > --- > libavformat/movenc.c | 28 +++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > index 10b959ad02..aa4a9c962a 100644 > --- a/libavformat/movenc.c > +++ b/libavformat/movenc.c > @@ -1028,9 +1028,31 @@ static int mov_write_audio_tag(AVFormatContext > *s, AVIOContext *pb, MOVMuxContex > avio_wb16(pb, 0); /* packet size (= 0) */ This patch is corrupted by line wraps / newlines [...]
2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>:
> + if (track->par->codec_id == AV_CODEC_ID_FLAC) {
Why does this only apply to flac?
Carl Eugen
Hi, On Fri, Aug 25, 2017 at 7:29 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Fri, Aug 25, 2017 at 01:25:23PM +0200, Jean-Yves Avenard wrote: > > From 9baa7166fa96ed6beac9146c7e3b4dcf425a67d0 Mon Sep 17 00:00:00 2001 > > From: Jean-Yves Avenard <jyavenard@mozilla.com> > > Date: Fri, 25 Aug 2017 13:11:28 +0200 > > Subject: [PATCH] Properly store sampling rate for FLAC in mp4 > > > > Fixes ticket #6609 > > > > Signed-off-by: Jean-Yves Avenard <jyavenard@mozilla.com> > > --- > > libavformat/movenc.c | 28 +++++++++++++++++++++++++--- > > 1 file changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > > index 10b959ad02..aa4a9c962a 100644 > > --- a/libavformat/movenc.c > > +++ b/libavformat/movenc.c > > @@ -1028,9 +1028,31 @@ static int mov_write_audio_tag(AVFormatContext > > *s, AVIOContext *pb, MOVMuxContex > > avio_wb16(pb, 0); /* packet size (= 0) */ > > This patch is corrupted by line wraps / newlines > An uncorrupted version was attached to #6609: https://trac.ffmpeg.org/attachment/ticket/6609/0001-Properly-store-sampling-rate-for-FLAC-in-mp4.patch Ronald
hi On 26 August 2017 at 12:08, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: > >> + if (track->par->codec_id == AV_CODEC_ID_FLAC) { > > Why does this only apply to flac? > Sorry, I had missed your reply. The specification on how sampling rate is to be stored should it be greater than INT16_MAX is a FLAC in ISOBMFF requirement: https://git.xiph.org/?p=flac.git;a=blob;f=doc/isoflac.txt;h=574df9f54a779fca2e62c726703fc7be199d4c05;hb=refs/heads/master#l165 It definitely could be applied to other codecs, but I haven't seen such requirements clearly defined. ISOBMFF only defines that AudioSampleEntryV1 should be used instead, in which case the sampling_rate is a 32 bits integer (ISO 14496-12 12.2.3.2) JY
2017-10-25 16:52 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: > hi > > On 26 August 2017 at 12:08, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: >> >>> + if (track->par->codec_id == AV_CODEC_ID_FLAC) { >> >> Why does this only apply to flac? >> > > Sorry, I had missed your reply. > > The specification on how sampling rate is to be stored should it be > greater than INT16_MAX is a FLAC in ISOBMFF requirement: > > https://git.xiph.org/?p=flac.git;a=blob;f=doc/isoflac.txt;h=574df9f54a779fca2e62c726703fc7be199d4c05;hb=refs/heads/master#l165 > > It definitely could be applied to other codecs, but I haven't seen > such requirements clearly defined. > > ISOBMFF only defines that AudioSampleEntryV1 should be used instead, > in which case the sampling_rate is a 32 bits integer (ISO 14496-12 > 12.2.3.2) Not sure I understand: In ticket #6609, I asked if this is a flac-only issue. You answered: "The issue can be reproduced with any codec." So why does your patch only fix the issue for flac? Or do I misunderstand? Thank you, Carl Eugen
Hi, On Wed, Oct 25, 2017 at 5:57 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-10-25 16:52 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: > > hi > > > > On 26 August 2017 at 12:08, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: > >> > >>> + if (track->par->codec_id == AV_CODEC_ID_FLAC) { > >> > >> Why does this only apply to flac? > >> > > > > Sorry, I had missed your reply. > > > > The specification on how sampling rate is to be stored should it be > > greater than INT16_MAX is a FLAC in ISOBMFF requirement: > > > > https://git.xiph.org/?p=flac.git;a=blob;f=doc/isoflac.txt;h= > 574df9f54a779fca2e62c726703fc7be199d4c05;hb=refs/heads/master#l165 > > > > It definitely could be applied to other codecs, but I haven't seen > > such requirements clearly defined. > > > > ISOBMFF only defines that AudioSampleEntryV1 should be used instead, > > in which case the sampling_rate is a 32 bits integer (ISO 14496-12 > > 12.2.3.2) > > Not sure I understand: > In ticket #6609, I asked if this is a flac-only issue. > You answered: "The issue can be reproduced with any codec." > > So why does your patch only fix the issue for flac? > Or do I misunderstand? > The extension for storing values greater than int16_max is only part of the isobmff-flac spec, it's not generalized for other audio codecs. Using it for other audio codecs may have unwanted results, which is why it's flac-specific. Do you want a comment in the source code to make this easier to understand for future developers staring at this code? Ronald
On Wed, Oct 25, 2017 at 06:48:06PM -0400, Ronald S. Bultje wrote: > Hi, > > On Wed, Oct 25, 2017 at 5:57 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > > > 2017-10-25 16:52 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: > > > hi > > > > > > On 26 August 2017 at 12:08, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > >> 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: > > >> > > >>> + if (track->par->codec_id == AV_CODEC_ID_FLAC) { > > >> > > >> Why does this only apply to flac? > > >> > > > > > > Sorry, I had missed your reply. > > > > > > The specification on how sampling rate is to be stored should it be > > > greater than INT16_MAX is a FLAC in ISOBMFF requirement: > > > > > > https://git.xiph.org/?p=flac.git;a=blob;f=doc/isoflac.txt;h= > > 574df9f54a779fca2e62c726703fc7be199d4c05;hb=refs/heads/master#l165 > > > > > > It definitely could be applied to other codecs, but I haven't seen > > > such requirements clearly defined. > > > > > > ISOBMFF only defines that AudioSampleEntryV1 should be used instead, > > > in which case the sampling_rate is a 32 bits integer (ISO 14496-12 > > > 12.2.3.2) > > > > Not sure I understand: > > In ticket #6609, I asked if this is a flac-only issue. > > You answered: "The issue can be reproduced with any codec." > > > > So why does your patch only fix the issue for flac? > > Or do I misunderstand? > > > > The extension for storing values greater than int16_max is only part of the > isobmff-flac spec, it's not generalized for other audio codecs. Using it > for other audio codecs may have unwanted results, which is why it's > flac-specific. > > Do you want a comment in the source code to make this easier to understand > for future developers staring at this code? the question wasnt intended for me, but yes [...]
Hi On 25 October 2017 at 23:57, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> ISOBMFF only defines that AudioSampleEntryV1 should be used instead, >> in which case the sampling_rate is a 32 bits integer (ISO 14496-12 >> 12.2.3.2) > > Not sure I understand: > In ticket #6609, I asked if this is a flac-only issue. > You answered: "The issue can be reproduced with any codec." The issue can be reproduced with every codec. That is, every mp4 file with an audio sampling rate greater than INT16_MAX will have 0 for the sample_rate value. This will prevent those generated files to be played in Firefox for example. A sampling rate of 0 being treated as invalid. This is why I answered that the problem could be reproduce with any codecs. It is a problem with all audio codec when used with FFmpeg generated mp4. However, only the flac-in-isobmff defines on what to do. If we were to generalize the fix to other codec, then those files wouldn't be spec compliant either (though it's my belief it would be better if it were). > > So why does your patch only fix the issue for flac? > Or do I misunderstand? Only the flac-in-isobmff clearly states what to do under those circumstances. For the other format, there's no specific documentation on what should be done for how to store sampling rate value greater than 16 bits. ISOBMFF spec itself define an AudioSampleEntryV1 box instead which has the sampling rate store on 32 bits instead. FFmpeg doesn't support AudioSampleEntryV1 So the special fix for flac is one poor man's attempt to make things better without having to implement a much more complex fix. (That and few players supports AudioSampleEntryV1 either) Hope this help clarify the problem at hand better. Sorry if I didn't make things clearer earlier. JY
2017-10-26 8:37 GMT+02:00 Jean-Yves Avenard <jyavenard@gmail.com>: > Hi > > On 25 October 2017 at 23:57, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >>> ISOBMFF only defines that AudioSampleEntryV1 should be used instead, >>> in which case the sampling_rate is a 32 bits integer (ISO 14496-12 >>> 12.2.3.2) >> >> Not sure I understand: >> In ticket #6609, I asked if this is a flac-only issue. >> You answered: "The issue can be reproduced with any codec." > > The issue can be reproduced with every codec. > > That is, every mp4 file with an audio sampling rate greater than > INT16_MAX will have 0 for the sample_rate value. > This will prevent those generated files to be played in Firefox > for example. Was this already mentioned somewhere? Do other codecs with large sample rates in mp4 play with Firefox? Carl Eugen
On 26 October 2017 at 16:34, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Was this already mentioned somewhere? > Do other codecs with large sample rates in mp4 play with Firefox? not if the metadata reports a sampling rate of 0....
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 10b959ad02..aa4a9c962a 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1028,9 +1028,31 @@ static int mov_write_audio_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContex avio_wb16(pb, 0); /* packet size (= 0) */ if (track->par->codec_id == AV_CODEC_ID_OPUS) avio_wb16(pb, 48000); - else - avio_wb16(pb, track->par->sample_rate <= UINT16_MAX ? - track->par->sample_rate : 0); + else { + uint32_t rate; + if (track->par->codec_id == AV_CODEC_ID_FLAC) { + /* When the bitstream's native sample rate is greater + than the maximum expressible value of 65535 Hz, + the samplerate field shall hold the greatest + expressible regular division of that rate. I.e. + the samplerate field shall hold 48000.0 for + native sample rates of 96 and 192 kHz. In the + case of unusual sample rates which do not have + an expressible regular division, the maximum value + of 65535.0 Hz should be used. */ + rate = track->par->sample_rate; + while (rate > UINT16_MAX && (rate & 1) == 0) { + rate = rate >> 1; + } + if (rate > UINT16_MAX) { + rate = UINT16_MAX; + } + } else { + rate = track->par->sample_rate <= UINT16_MAX ? + track->par->sample_rate : 0; + } + avio_wb16(pb, rate); + } avio_wb16(pb, 0); /* Reserved */ }