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

[Andreas Rheinhardt]

Michael Niedermayer:
> On Thu, May 30, 2024 at 08:07:48PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Thu, May 30, 2024 at 07:53:42PM +0200, Andreas Rheinhardt wrote:
>>>> 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.
>>>
>>> i honestly belive that we should support changing channel(layout) for
>>> cases like PCM in nut
>>>
>>
>> That is orthogonal to this patch (which just wants to not create
>> inconsistent side data).
> 
> You can fix the inconsistency in 2 directions
> 1. remove everyting
> 2. add the code back that made it inconsistant
> 
> This line between these 2 points is not orthogonal to what this patch changes
> It also is not orthoginal to supporting PCM channel changes in NUT
> nor is the change this patch does from our current state orthogonal
> to what would be needed to support channel changes
> 
> IMHO, decide on what the end goal is and work toward it. Not just
> make something consistent even when its a direction that might be suboptimal
> 

We have a release that is able to create nonsense side data; this needs
to be fixed and for this to be fixed we need a patch to backport.

If you think that we also need a way to signal channel layout/count
updates, then you can propose such a thing afterwards.

- Andreas