Message ID | 9bc3ecc2-109e-3c8f-f5a7-e102a29e731d@noa-archive.com |
---|---|
State | New |
Headers | show |
On Wed, Oct 04, 2017 at 10:33:11AM +0200, Tobias Rapp wrote: > On 30.09.2017 02:48, Michael Niedermayer wrote: > >On Fri, Sep 29, 2017 at 05:08:16PM +0200, Tobias Rapp wrote: > >>Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com> > >>--- > >> libavformat/wavenc.c | 5 ++++- > >> tests/ref/lavf/wav_peak_only | 2 +- > >> 2 files changed, 5 insertions(+), 2 deletions(-) > > > >The commit message doest say why this chnage is done > > As I understand the BWF peak envelope chunk definition in EBU Tech > 3285 - Supplement 3 the "dwPosPeakOfPeaks" field contains the > (absolute) byte position of the peak audio sample within the _data_ > chunk: > > """ > The peak-of-peaks is first audio sample whose absolute value is the > maximum value of the entire audio file. > > Rather than storing the peak-of-peaks as a sample value, the > position of the peak of the peaks is stored. In other words, an > audio sample frame index is stored. An application then knows where > to read the peak of the peaks in the audio file. It would be more > difficult to store a value for peak since this is dependant on the > binary format of the audio > samples (integers, floats, double...). > > If the value is 0xFFFFFFFF, then that means that the peak of the > peaks is unknown. > """ > > As a peak-only file doesn't contain a data chunk it would make no > sense to store the data position. > > But when looking at FFmpeg's implementation within wavenc.c I notice > now, that the implementation is using a totally different > interpretation of the spec and stores the peak frame index (position > relative to peak chunk data) instead. > > In my opinion the current implementation of "dwPosPeakOfPeaks" is > wrong and the patch should actually be changed to always write "-1" > unconditionally until the peak-of-peaks feature is implemented > correctly. See attached patch update. have you confirmed that files not created by ffmpeg mismatch what we do ? If so this is ok but please bump the micro version of libavformat every time this muxer behavior changes. So demuxers can know what they are dealing with [...]
On 05.10.2017 00:05, Michael Niedermayer wrote: > On Wed, Oct 04, 2017 at 10:33:11AM +0200, Tobias Rapp wrote: >> On 30.09.2017 02:48, Michael Niedermayer wrote: >>> On Fri, Sep 29, 2017 at 05:08:16PM +0200, Tobias Rapp wrote: >>>> Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com> >>>> --- >>>> libavformat/wavenc.c | 5 ++++- >>>> tests/ref/lavf/wav_peak_only | 2 +- >>>> 2 files changed, 5 insertions(+), 2 deletions(-) >>> >>> The commit message doest say why this chnage is done >> >> As I understand the BWF peak envelope chunk definition in EBU Tech >> 3285 - Supplement 3 the "dwPosPeakOfPeaks" field contains the >> (absolute) byte position of the peak audio sample within the _data_ >> chunk: >> >> """ >> The peak-of-peaks is first audio sample whose absolute value is the >> maximum value of the entire audio file. >> >> Rather than storing the peak-of-peaks as a sample value, the >> position of the peak of the peaks is stored. In other words, an >> audio sample frame index is stored. An application then knows where >> to read the peak of the peaks in the audio file. It would be more >> difficult to store a value for peak since this is dependant on the >> binary format of the audio >> samples (integers, floats, double...). >> >> If the value is 0xFFFFFFFF, then that means that the peak of the >> peaks is unknown. >> """ >> >> As a peak-only file doesn't contain a data chunk it would make no >> sense to store the data position. >> >> But when looking at FFmpeg's implementation within wavenc.c I notice >> now, that the implementation is using a totally different >> interpretation of the spec and stores the peak frame index (position >> relative to peak chunk data) instead. >> >> In my opinion the current implementation of "dwPosPeakOfPeaks" is >> wrong and the patch should actually be changed to always write "-1" >> unconditionally until the peak-of-peaks feature is implemented >> correctly. See attached patch update. > > have you confirmed that files not created by ffmpeg mismatch what we > do ? > > If so this is ok but > please bump the micro version of libavformat every time > this muxer behavior changes. So demuxers can know what they are dealing > with Actually our own software (NOA MediaButler) always sets this field to "-1" when writing BWF files exactly due to the hassle of calculating the value. And when reading a Wave file for normalization the peak-of-peaks is re-calculated within the target time-range anyway, so this field is ignored. Unfortunately I don't have access to other software that writes "levl" chunks but am putting Georg Lippitsch on CC as he did the implementation of BWF features within wavenc.c . I don't care if this patch is dropped, just stumbled over it when comparing FFmpeg BWF output to files of our own software and to the BWF specs. Regards, Tobias
diff --git a/libavformat/wavenc.c b/libavformat/wavenc.c index adb20cb..a6060de 100644 --- a/libavformat/wavenc.c +++ b/libavformat/wavenc.c @@ -74,7 +74,6 @@ typedef struct WAVMuxContext { uint32_t peak_num_frames; uint32_t peak_outbuf_size; uint32_t peak_outbuf_bytes; - uint32_t peak_pos_pop; uint16_t peak_pop; uint8_t *peak_output; int last_duration; @@ -215,8 +214,6 @@ static void peak_write_frame(AVFormatContext *s) peak_of_peaks = FFMAX3(wav->peak_maxpos[c], wav->peak_maxneg[c], wav->peak_pop); - if (peak_of_peaks > wav->peak_pop) - wav->peak_pos_pop = wav->peak_num_frames; wav->peak_pop = peak_of_peaks; if (wav->peak_outbuf_size - wav->peak_outbuf_bytes < @@ -287,7 +284,7 @@ static int peak_write_chunk(AVFormatContext *s) avio_wl32(pb, wav->peak_block_size); /* frames per value */ avio_wl32(pb, par->channels); /* number of channels */ avio_wl32(pb, wav->peak_num_frames); /* number of peak frames */ - avio_wl32(pb, wav->peak_pos_pop); /* audio sample frame index */ + avio_wl32(pb, -1); /* audio sample frame position (TODO) */ avio_wl32(pb, 128); /* equal to size of header */ avio_write(pb, timestamp, 28); /* ASCII time stamp */ ffio_fill(pb, 0, 60); diff --git a/tests/ref/lavf/wav_peak b/tests/ref/lavf/wav_peak index aa7e5fc..861b246 100644 --- a/tests/ref/lavf/wav_peak +++ b/tests/ref/lavf/wav_peak @@ -1,3 +1,3 @@ -35148d1f6e66b0080893851d917ecbf4 *./tests/data/lavf/lavf.peak.wav +105805963fb767d00da056f42f32d9f3 *./tests/data/lavf/lavf.peak.wav 89094 ./tests/data/lavf/lavf.peak.wav ./tests/data/lavf/lavf.peak.wav CRC=0x3a1da17e diff --git a/tests/ref/lavf/wav_peak_only b/tests/ref/lavf/wav_peak_only index dccd0e7..b203d03 100644 --- a/tests/ref/lavf/wav_peak_only +++ b/tests/ref/lavf/wav_peak_only @@ -1,2 +1,2 @@ -b609a363e6d490710ed52231a8d09d3c *./tests/data/lavf/lavf.peak_only.wav +f1a8aeeae8069f3992c4d780436c3d23 *./tests/data/lavf/lavf.peak_only.wav 832 ./tests/data/lavf/lavf.peak_only.wav