| Message ID | AS8P250MB07449DE65147D761FDC562BD8FF32@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 87a13986bc08627dc847f4475bc91ee7c78618de |
| Headers | show |
| Series | [FFmpeg-devel,v3] avformat/nutdec: Don't create inconsistent side data | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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 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
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 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
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 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
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 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
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 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
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
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){
Forgotten in 65ddc74988245a01421a63c5cffa4d900c47117c. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavformat/nutdec.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)