Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
On 26 October 2017 at 16:34, Carl Eugen Hoyoswrote: > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
2017-10-26 8:37 GMT+02:00 Jean-Yves Avenard: > Hi > > On 25 October 2017 at 23:57, Carl Eugen Hoyos 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
Hi On 25 October 2017 at 23:57, Carl Eugen Hoyoswrote: >> 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
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> wrote: > > > 2017-10-25 16:52 GMT+02:00 Jean-Yves Avenard : > > > hi > > > > > > On 26 August 2017 at 12:08, Carl Eugen Hoyos wrote: > > >> 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard : > > >> > > >>> +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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When you are offended at any man's fault, turn to yourself and study your own failings. Then you will forget your anger. -- Epictetus signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
Hi, On Wed, Oct 25, 2017 at 5:57 PM, Carl Eugen Hoyoswrote: > 2017-10-25 16:52 GMT+02:00 Jean-Yves Avenard : > > hi > > > > On 26 August 2017 at 12:08, Carl Eugen Hoyos wrote: > >> 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard : > >> > >>> +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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
2017-10-25 16:52 GMT+02:00 Jean-Yves Avenard: > hi > > On 26 August 2017 at 12:08, Carl Eugen Hoyos wrote: >> 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard : >> >>> +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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
hi On 26 August 2017 at 12:08, Carl Eugen Hoyoswrote: > 2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard : > >> +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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
Hi, On Fri, Aug 25, 2017 at 7:29 PM, Michael Niedermayerwrote: > 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 > > 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 > > --- > > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
2017-08-25 13:25 GMT+02:00 Jean-Yves Avenard: > +if (track->par->codec_id == AV_CODEC_ID_FLAC) { Why does this only apply to flac? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
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> 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 > --- > 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Take away the freedom of one citizen and you will be jailed, take away the freedom of all citizens and you will be congratulated by your peers in Parliament. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
From 9baa7166fa96ed6beac9146c7e3b4dcf425a67d0 Mon Sep 17 00:00:00 2001 From: Jean-Yves AvenardDate: 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 --- 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) */ 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 */ } -- 2.11.0 (Apple Git-81) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Properly store sampling rate for FLAC in mp4
Note that that it's also not correct for other codecs when the sampling rate is greater than 65536. Right now it stores 0. If in quicktime mode, it could use a SoundDescription v2 box, but in mp4 that doesn't exist. Per ISO 14496-12 , it should be using a AudioSampleEntryV1 along a SamplingRateBox ('srat') which uses a 32 bits unsigned integer. There's no handling of that in the current ffmpeg. If time permit I will submit something for this. Rgds JY On 25 August 2017 at 13:25, Jean-Yves Avenardwrote: > From 9baa7166fa96ed6beac9146c7e3b4dcf425a67d0 Mon Sep 17 00:00:00 2001 > From: Jean-Yves Avenard > 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 > --- > 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) */ > 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 */ > } > > -- > 2.11.0 (Apple Git-81) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel