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