diff mbox

[FFmpeg-devel,2/2] avformat/wavenc: skip writing peak-of-peaks position when writing peaks only

Message ID 9bc3ecc2-109e-3c8f-f5a7-e102a29e731d@noa-archive.com
State New
Headers show

Commit Message

Tobias Rapp Oct. 4, 2017, 8:33 a.m. UTC 
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.

Regards,
Tobias
From 07a2414a1154dd51b9da09bbd2c877363860e825 Mon Sep 17 00:00:00 2001
From: Tobias Rapp <t.rapp@noa-archive.com>
Date: Fri, 29 Sep 2017 16:32:20 +0200
Subject: [PATCH v2 2/2] avformat/wavenc: skip writing incorrect peak-of-peaks
 position value

According to EBU tech 3285 supplement 3 the dwPosPeakOfPeaks field
should contain the absolute position to the maximum audio sample value,
but the current implementation writes the relative peak frame index
instead.

Fix the issue by writing the "unknown" value (-1) instead for now until
the feature is implemented correctly.

Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
---
 libavformat/wavenc.c         | 5 +----
 tests/ref/lavf/wav_peak      | 2 +-
 tests/ref/lavf/wav_peak_only | 2 +-
 3 files changed, 3 insertions(+), 6 deletions(-)

Comments

Michael Niedermayer Oct. 4, 2017, 10:05 p.m. UTC | #1 
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

[...]
Tobias Rapp Oct. 5, 2017, 7:47 a.m. UTC | #2 
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 mbox

Patch

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