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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 7407 bytes --]

On Fri, May 31, 2024 at 12:01:56AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Thu, May 30, 2024 at 09:33:51PM +0200, Andreas Rheinhardt wrote:
> >> 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@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.
> > 
> > what do you mean by "nonsense side data" ?
> > 
> > I would assume the side data is what was documented previously
> > 
> 
> 65ddc74988245a01421a63c5cffa4d900c47117c removed the code that set the
> bits indicating that new channel layout/count data is present (because
> the flags have been removed); yet it did not remove the code actually
> writing said fields. This means that if there is a new channel layout
> (or simply the old one repeated) and a new sample rate, the demuxer and
> a user/reader have differing opinions about the side data: The demuxer
> thinks it wrote the new sample rate at offset 12, a reader thinks it is
> at offset 4. That is the inconsistency the commit message refers to.

you could revert the related hunks, that would be in line with fixing
the inconsistancy

Or if you want a light hearted comparission
consider driving your car over a nail, the right front tire looses its air
so you fix that by picking up the nail and hammering it in the left rear tire
This is comparable as we still support changing sample rate per side data
and we also support changing channels when its initiated by the decoder

To me the removal of the mid stream channel change support simply looks
like a mistake.
Iam also not sure why it was under FF_API_OLD_CHANNEL_LAYOUT. The mid stream
change code surely could have been updated to use the new API with everything
else

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".