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.rheinha...@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-requ...@ffmpeg.org with subject "unsubscribe".
