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

[Tobias Rapp]

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 at 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
-------------- next part --------------
From 07a2414a1154dd51b9da09bbd2c877363860e825 Mon Sep 17 00:00:00 2001
From: Tobias Rapp <t.rapp at 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 at 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(-)

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