[FFmpeg-devel] [PATCH v3] avformat/nutdec: Don't create inconsistent side data
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Fri May 31 23:30:41 EEST 2024
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 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
>>>>>
>>>>
>>>> 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
More information about the ffmpeg-devel
mailing list