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

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

[Andreas Rheinhardt]

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){
-- 
2.40.1

_______________________________________________
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".

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

[James Almer]

On 5/29/2024 9:14 PM, 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){

LGTM. Should be backported too.
_______________________________________________
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".

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

[Michael Niedermayer]

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

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

thx

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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad

[-- 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".

Re: [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@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

_______________________________________________
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".

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

[Michael Niedermayer]

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

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire

[-- 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".

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

[Andreas Rheinhardt]

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).

- Andreas

_______________________________________________
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".

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

[Michael Niedermayer]

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

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

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

[-- 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".

Re: [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@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

_______________________________________________
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".

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

[Michael Niedermayer]

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

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

thx

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 

[-- 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".

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

[Andreas Rheinhardt]

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.

- Andreas

_______________________________________________
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".

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

[Michael Niedermayer]

[-- 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".

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

[Andreas Rheinhardt]

Michael Niedermayer:
> 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

1. Simply reverting the related hunks is not enough: All users who
support this side data and who support git master (or the latest
release) have dropped their code for channel count/layout changes,
therefore the channel count/layout fields (if resurrected) would need to
be added at the end of AV_PKT_DATA_PARAM_CHANGE. So if this is readded,
it would have to be readded in a slightly different form.
2. This also shows that the side data as currently (potentially)
produced is wrong according to the current documentation and according
to any fixed documentation. It should therefore be removed (and the fix
backported) as I intend to do.
3. As I said: If you think that 09b5d3fb44ae10 was a mistake, you can
send a patch for this afterwards. But I don't see a reason to bikeshed
this patch here.
4. I think the reason why this has been deprecated with
FF_API_OLD_CHANNEL_LAYOUT was that no codec used it, so Anton etc. did
not see a need to update it to the new API. But you should better ask
them this.

- Andreas

_______________________________________________
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".

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

[Andreas Rheinhardt]

Andreas Rheinhardt:
> 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){

Will apply this patch tomorrow unless there are objections.

- Andreas

_______________________________________________
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".