[FFmpeg-devel] [PATCH v3] avformat/nutdec: Don't create inconsistent side data

[Andreas Rheinhardt]

Michael Niedermayer:
> On Thu, May 30, 2024 at 02:14:20AM +0200, Andreas Rheinhardt wrote:
>> Forgotten in 65ddc74988245a01421a63c5cffa4d900c47117c.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>>  libavformat/nutdec.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
>> index 0bb7f154db..34b7e3cb9a 100644
>> --- a/libavformat/nutdec.c
>> +++ b/libavformat/nutdec.c
>> @@ -881,8 +881,6 @@ static int read_sm_data(AVFormatContext *s, AVIOContext *bc, AVPacket *pkt, int
>>      int count = ffio_read_varlen(bc);
>>      int skip_start = 0;
>>      int skip_end = 0;
>> -    int channels = 0;
>> -    int64_t channel_layout = 0;
>>      int sample_rate = 0;
>>      int width = 0;
>>      int height = 0;
>> @@ -930,7 +928,7 @@ static int read_sm_data(AVFormatContext *s, AVIOContext *bc, AVPacket *pkt, int
>>                  AV_WB64(dst, v64);
>>                  dst += 8;
>>              } else if (!strcmp(name, "ChannelLayout") && value_len == 8) {
>> -                channel_layout = avio_rl64(bc);
>> +                // Ignored
>>                  continue;
>>              } else {
>>                  av_log(s, AV_LOG_WARNING, "Unknown data %s / %s\n", name, type_str);
>> @@ -952,7 +950,7 @@ static int read_sm_data(AVFormatContext *s, AVIOContext *bc, AVPacket *pkt, int
>>              } else if (!strcmp(name, "SkipEnd")) {
>>                  skip_end = value;
>>              } else if (!strcmp(name, "Channels")) {
>> -                channels = value;
>> +                // Ignored
>>              } else if (!strcmp(name, "SampleRate")) {
>>                  sample_rate = value;
>>              } else if (!strcmp(name, "Width")) {
>> @@ -965,18 +963,14 @@ static int read_sm_data(AVFormatContext *s, AVIOContext *bc, AVPacket *pkt, int
>>          }
>>      }
>>  
>> -    if (channels || channel_layout || sample_rate || width || height) {
>> -        uint8_t *dst = av_packet_new_side_data(pkt, AV_PKT_DATA_PARAM_CHANGE, 28);
>> +    if (sample_rate || width || height) {
>> +        uint8_t *dst = av_packet_new_side_data(pkt, AV_PKT_DATA_PARAM_CHANGE, 16);
>>          if (!dst)
>>              return AVERROR(ENOMEM);
>>          bytestream_put_le32(&dst,
>>                              AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE*(!!sample_rate) +
>>                              AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS*(!!(width|height))
>>                             );
>> -        if (channels)
>> -            bytestream_put_le32(&dst, channels);
>> -        if (channel_layout)
>> -            bytestream_put_le64(&dst, channel_layout);
>>          if (sample_rate)
>>              bytestream_put_le32(&dst, sample_rate);
>>          if (width || height){
> 
> This would break mid stream changes to the channel layout & channels when it
> is carried at format level only
> 
> The commit message also does not adequately explain why such mid stream changes
> are ignored
> 

Mid-stream changes like this have been deprecated in
09b5d3fb44ae1036700f80c8c80b15e9074c58c3;
65ddc74988245a01421a63c5cffa4d900c47117c removed it, but only
incompletely: The side data flags for channel count and channel layout
changes were no longer written (in fact, they were removed from
packet.h), yet it still wrote the rest of the side data as if these
flags existed and had been written. That is the inconsistency this
commit addresses. It does not address whether channel count/layout
updates should have been removed, because that has already happened.

- Andreas