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

[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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240530/89dbb428/attachment.sig>