| Message ID | 20180718173146.247064-1-gorzel@google.com |
|---|---|
| State | New |
| Headers | show |
On 18.07.2018 19:31, Marcin Gorzel wrote: > Rematrixing supports up to 64 channels. However, there is only a limited number of channel layouts defined. Since the in/out channel count is obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 channels etc.) the rematrixing fails. > > In ticket #6790 the problem has been partially addressed by applying a patch to swr_set_matrix() in rematrix.c:72. > However, a similar change must be also applied to swri_rematrix_init() in rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36. > --- > libswresample/rematrix.c | 6 ++++-- > libswresample/x86/rematrix_init.c | 6 ++++-- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > index 8227730056..d1a0cfe7f8 100644 > --- a/libswresample/rematrix.c > +++ b/libswresample/rematrix.c > @@ -384,8 +384,10 @@ av_cold static int auto_matrix(SwrContext *s) > > av_cold int swri_rematrix_init(SwrContext *s){ > int i, j; > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > + av_get_channel_layout_nb_channels(s->in_ch_layout); > + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > + av_get_channel_layout_nb_channels(s->out_ch_layout); > > s->mix_any_f = NULL; > > diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c > index d71b41a73e..4f9c92e4ab 100644 > --- a/libswresample/x86/rematrix_init.c > +++ b/libswresample/x86/rematrix_init.c > @@ -33,8 +33,10 @@ D(int16, sse2) > av_cold int swri_rematrix_init_x86(struct SwrContext *s){ > #if HAVE_X86ASM > int mm_flags = av_get_cpu_flags(); > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > + av_get_channel_layout_nb_channels(s->in_ch_layout); > + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > + av_get_channel_layout_nb_channels(s->out_ch_layout); > int num = nb_in * nb_out; > int i,j; > > Patch looks good to me, except that the title should be updated to reflect the new logic. I suggest something like "swresample: Use channel count for rematrix initialization if defined". If nobody objects within the next days I will amend the title, push the patch to master and backport the fix to release branches. Regards, Tobias
Hi Tobias, Sounds good, thanks a lot! Regards, Marcin On Thu, Jul 19, 2018 at 12:53 PM Tobias Rapp <t.rapp@noa-archive.com> wrote: > On 18.07.2018 19:31, Marcin Gorzel wrote: > > Rematrixing supports up to 64 channels. However, there is only a limited > number of channel layouts defined. Since the in/out channel count is > obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 > channels etc.) the rematrixing fails. > > > > In ticket #6790 the problem has been partially addressed by applying a > patch to swr_set_matrix() in rematrix.c:72. > > However, a similar change must be also applied to swri_rematrix_init() > in rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36. > > --- > > libswresample/rematrix.c | 6 ++++-- > > libswresample/x86/rematrix_init.c | 6 ++++-- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > > index 8227730056..d1a0cfe7f8 100644 > > --- a/libswresample/rematrix.c > > +++ b/libswresample/rematrix.c > > @@ -384,8 +384,10 @@ av_cold static int auto_matrix(SwrContext *s) > > > > av_cold int swri_rematrix_init(SwrContext *s){ > > int i, j; > > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > > + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > > + av_get_channel_layout_nb_channels(s->in_ch_layout); > > + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > > + av_get_channel_layout_nb_channels(s->out_ch_layout); > > > > s->mix_any_f = NULL; > > > > diff --git a/libswresample/x86/rematrix_init.c > b/libswresample/x86/rematrix_init.c > > index d71b41a73e..4f9c92e4ab 100644 > > --- a/libswresample/x86/rematrix_init.c > > +++ b/libswresample/x86/rematrix_init.c > > @@ -33,8 +33,10 @@ D(int16, sse2) > > av_cold int swri_rematrix_init_x86(struct SwrContext *s){ > > #if HAVE_X86ASM > > int mm_flags = av_get_cpu_flags(); > > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > > + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > > + av_get_channel_layout_nb_channels(s->in_ch_layout); > > + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > > + av_get_channel_layout_nb_channels(s->out_ch_layout); > > int num = nb_in * nb_out; > > int i,j; > > > > > > Patch looks good to me, except that the title should be updated to > reflect the new logic. I suggest something like "swresample: Use channel > count for rematrix initialization if defined". > > If nobody objects within the next days I will amend the title, push the > patch to master and backport the fix to release branches. > > Regards, > Tobias > >
On Thu, Jul 19, 2018 at 01:53:09PM +0200, Tobias Rapp wrote: > On 18.07.2018 19:31, Marcin Gorzel wrote: > >Rematrixing supports up to 64 channels. However, there is only a limited number of channel layouts defined. Since the in/out channel count is obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 channels etc.) the rematrixing fails. > > > >In ticket #6790 the problem has been partially addressed by applying a patch to swr_set_matrix() in rematrix.c:72. > >However, a similar change must be also applied to swri_rematrix_init() in rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36. > >--- > > libswresample/rematrix.c | 6 ++++-- > > libswresample/x86/rematrix_init.c | 6 ++++-- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > >diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > >index 8227730056..d1a0cfe7f8 100644 > >--- a/libswresample/rematrix.c > >+++ b/libswresample/rematrix.c > >@@ -384,8 +384,10 @@ av_cold static int auto_matrix(SwrContext *s) > > av_cold int swri_rematrix_init(SwrContext *s){ > > int i, j; > >- int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > >- int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > >+ int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > >+ av_get_channel_layout_nb_channels(s->in_ch_layout); > >+ int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > >+ av_get_channel_layout_nb_channels(s->out_ch_layout); > > s->mix_any_f = NULL; > >diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c > >index d71b41a73e..4f9c92e4ab 100644 > >--- a/libswresample/x86/rematrix_init.c > >+++ b/libswresample/x86/rematrix_init.c > >@@ -33,8 +33,10 @@ D(int16, sse2) > > av_cold int swri_rematrix_init_x86(struct SwrContext *s){ > > #if HAVE_X86ASM > > int mm_flags = av_get_cpu_flags(); > >- int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > >- int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > >+ int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > >+ av_get_channel_layout_nb_channels(s->in_ch_layout); > >+ int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > >+ av_get_channel_layout_nb_channels(s->out_ch_layout); > > int num = nb_in * nb_out; > > int i,j; > > > > Patch looks good to me, except that the title should be updated to reflect > the new logic. I suggest something like "swresample: Use channel count for > rematrix initialization if defined". The patch does not look good to me There should be a field that represents the count of channels. No conditional should be needed please check that every field is wrong in at least some case. If true a new field must be added or a existing one needs to be set differently But there should be a field that represents the channel count. [...]
On 19.07.2018 23:37, Michael Niedermayer wrote: > On Thu, Jul 19, 2018 at 01:53:09PM +0200, Tobias Rapp wrote: >> On 18.07.2018 19:31, Marcin Gorzel wrote: >>> Rematrixing supports up to 64 channels. However, there is only a limited number of channel layouts defined. Since the in/out channel count is obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 channels etc.) the rematrixing fails. >>> >>> In ticket #6790 the problem has been partially addressed by applying a patch to swr_set_matrix() in rematrix.c:72. >>> However, a similar change must be also applied to swri_rematrix_init() in rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36. >>> --- >>> libswresample/rematrix.c | 6 ++++-- >>> libswresample/x86/rematrix_init.c | 6 ++++-- >>> 2 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c >>> index 8227730056..d1a0cfe7f8 100644 >>> --- a/libswresample/rematrix.c >>> +++ b/libswresample/rematrix.c >>> @@ -384,8 +384,10 @@ av_cold static int auto_matrix(SwrContext *s) >>> av_cold int swri_rematrix_init(SwrContext *s){ >>> int i, j; >>> - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); >>> - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); >>> + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : >>> + av_get_channel_layout_nb_channels(s->in_ch_layout); >>> + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : >>> + av_get_channel_layout_nb_channels(s->out_ch_layout); >>> s->mix_any_f = NULL; >>> diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c >>> index d71b41a73e..4f9c92e4ab 100644 >>> --- a/libswresample/x86/rematrix_init.c >>> +++ b/libswresample/x86/rematrix_init.c >>> @@ -33,8 +33,10 @@ D(int16, sse2) >>> av_cold int swri_rematrix_init_x86(struct SwrContext *s){ >>> #if HAVE_X86ASM >>> int mm_flags = av_get_cpu_flags(); >>> - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); >>> - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); >>> + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : >>> + av_get_channel_layout_nb_channels(s->in_ch_layout); >>> + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : >>> + av_get_channel_layout_nb_channels(s->out_ch_layout); >>> int num = nb_in * nb_out; >>> int i,j; >>> >> >> Patch looks good to me, except that the title should be updated to reflect >> the new logic. I suggest something like "swresample: Use channel count for >> rematrix initialization if defined". > > The patch does not look good to me > There should be a field that represents the count of channels. > No conditional should be needed > > please check that every field is wrong in at least some case. > If true a new field must be added or a existing one needs to be set > differently > But there should be a field that represents the channel count. If I understand correctly you say that the fall-back to av_get_channel_layout_nb_channels() is not needed here as both functions are internal and called only as part of swr_init() so we may safely assume that the in/out.ch_count fields have been initialized before this code is reached. Fine with me. Marcin, would you update the patch once more? And there are some additional FATE tests for the pan filter that I can post once this patch is through, if you haven't made up your own tests. Regards, Tobias
On Fri, Jul 20, 2018 at 09:51:49AM +0200, Tobias Rapp wrote: > On 19.07.2018 23:37, Michael Niedermayer wrote: > >On Thu, Jul 19, 2018 at 01:53:09PM +0200, Tobias Rapp wrote: > >>On 18.07.2018 19:31, Marcin Gorzel wrote: > >>>Rematrixing supports up to 64 channels. However, there is only a limited number of channel layouts defined. Since the in/out channel count is obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 channels etc.) the rematrixing fails. > >>> > >>>In ticket #6790 the problem has been partially addressed by applying a patch to swr_set_matrix() in rematrix.c:72. > >>>However, a similar change must be also applied to swri_rematrix_init() in rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36. > >>>--- > >>> libswresample/rematrix.c | 6 ++++-- > >>> libswresample/x86/rematrix_init.c | 6 ++++-- > >>> 2 files changed, 8 insertions(+), 4 deletions(-) > >>> > >>>diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > >>>index 8227730056..d1a0cfe7f8 100644 > >>>--- a/libswresample/rematrix.c > >>>+++ b/libswresample/rematrix.c > >>>@@ -384,8 +384,10 @@ av_cold static int auto_matrix(SwrContext *s) > >>> av_cold int swri_rematrix_init(SwrContext *s){ > >>> int i, j; > >>>- int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > >>>- int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > >>>+ int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > >>>+ av_get_channel_layout_nb_channels(s->in_ch_layout); > >>>+ int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > >>>+ av_get_channel_layout_nb_channels(s->out_ch_layout); > >>> s->mix_any_f = NULL; > >>>diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c > >>>index d71b41a73e..4f9c92e4ab 100644 > >>>--- a/libswresample/x86/rematrix_init.c > >>>+++ b/libswresample/x86/rematrix_init.c > >>>@@ -33,8 +33,10 @@ D(int16, sse2) > >>> av_cold int swri_rematrix_init_x86(struct SwrContext *s){ > >>> #if HAVE_X86ASM > >>> int mm_flags = av_get_cpu_flags(); > >>>- int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > >>>- int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > >>>+ int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > >>>+ av_get_channel_layout_nb_channels(s->in_ch_layout); > >>>+ int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > >>>+ av_get_channel_layout_nb_channels(s->out_ch_layout); > >>> int num = nb_in * nb_out; > >>> int i,j; > >>> > >> > >>Patch looks good to me, except that the title should be updated to reflect > >>the new logic. I suggest something like "swresample: Use channel count for > >>rematrix initialization if defined". > > > >The patch does not look good to me > >There should be a field that represents the count of channels. > >No conditional should be needed > > > >please check that every field is wrong in at least some case. > >If true a new field must be added or a existing one needs to be set > >differently > >But there should be a field that represents the channel count. > > If I understand correctly you say that the fall-back to > av_get_channel_layout_nb_channels() is not needed here as both functions are > internal and called only as part of swr_init() so we may safely assume that > the in/out.ch_count fields have been initialized before this code is > reached. yes, if that is tested and works. If it does not work, it would be very interresting why not > > Fine with me. Marcin, would you update the patch once more? And there are > some additional FATE tests for the pan filter that I can post once this > patch is through, if you haven't made up your own tests. > > Regards, > Tobias > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Of course, I will update the patch and send it out shortly. Using in.ch_count should be ok. The relevant check is in swresample.c:292, so we should never reach that code if the in.ch_count is 0. On Fri, Jul 20, 2018 at 7:36 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Jul 20, 2018 at 09:51:49AM +0200, Tobias Rapp wrote: > > On 19.07.2018 23:37, Michael Niedermayer wrote: > > >On Thu, Jul 19, 2018 at 01:53:09PM +0200, Tobias Rapp wrote: > > >>On 18.07.2018 19:31, Marcin Gorzel wrote: > > >>>Rematrixing supports up to 64 channels. However, there is only a > limited number of channel layouts defined. Since the in/out channel count > is obtained from the channel layout, for undefined layouts (e.g. for 9, 10, > 11 channels etc.) the rematrixing fails. > > >>> > > >>>In ticket #6790 the problem has been partially addressed by applying > a patch to swr_set_matrix() in rematrix.c:72. > > >>>However, a similar change must be also applied to > swri_rematrix_init() in rematrix.c:389 and swri_rematrix_init_x86() in > rematrix_init.c:36. > > >>>--- > > >>> libswresample/rematrix.c | 6 ++++-- > > >>> libswresample/x86/rematrix_init.c | 6 ++++-- > > >>> 2 files changed, 8 insertions(+), 4 deletions(-) > > >>> > > >>>diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > > >>>index 8227730056..d1a0cfe7f8 100644 > > >>>--- a/libswresample/rematrix.c > > >>>+++ b/libswresample/rematrix.c > > >>>@@ -384,8 +384,10 @@ av_cold static int auto_matrix(SwrContext *s) > > >>> av_cold int swri_rematrix_init(SwrContext *s){ > > >>> int i, j; > > >>>- int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > > >>>- int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > > >>>+ int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > > >>>+ av_get_channel_layout_nb_channels(s->in_ch_layout); > > >>>+ int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > > >>>+ av_get_channel_layout_nb_channels(s->out_ch_layout); > > >>> s->mix_any_f = NULL; > > >>>diff --git a/libswresample/x86/rematrix_init.c > b/libswresample/x86/rematrix_init.c > > >>>index d71b41a73e..4f9c92e4ab 100644 > > >>>--- a/libswresample/x86/rematrix_init.c > > >>>+++ b/libswresample/x86/rematrix_init.c > > >>>@@ -33,8 +33,10 @@ D(int16, sse2) > > >>> av_cold int swri_rematrix_init_x86(struct SwrContext *s){ > > >>> #if HAVE_X86ASM > > >>> int mm_flags = av_get_cpu_flags(); > > >>>- int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > > >>>- int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > > >>>+ int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : > > >>>+ av_get_channel_layout_nb_channels(s->in_ch_layout); > > >>>+ int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : > > >>>+ av_get_channel_layout_nb_channels(s->out_ch_layout); > > >>> int num = nb_in * nb_out; > > >>> int i,j; > > >>> > > >> > > >>Patch looks good to me, except that the title should be updated to > reflect > > >>the new logic. I suggest something like "swresample: Use channel count > for > > >>rematrix initialization if defined". > > > > > >The patch does not look good to me > > >There should be a field that represents the count of channels. > > >No conditional should be needed > > > > > >please check that every field is wrong in at least some case. > > >If true a new field must be added or a existing one needs to be set > > >differently > > >But there should be a field that represents the channel count. > > > > If I understand correctly you say that the fall-back to > > av_get_channel_layout_nb_channels() is not needed here as both functions > are > > internal and called only as part of swr_init() so we may safely assume > that > > the in/out.ch_count fields have been initialized before this code is > > reached. > > yes, if that is tested and works. If it does not work, it would be very > interresting why not > > > > > > Fine with me. Marcin, would you update the patch once more? And there are > > some additional FATE tests for the pan filter that I can post once this > > patch is through, if you haven't made up your own tests. > > > > Regards, > > Tobias > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > "I am not trying to be anyone's saviour, I'm trying to think about the > future and not be sad" - Elon Musk > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c index 8227730056..d1a0cfe7f8 100644 --- a/libswresample/rematrix.c +++ b/libswresample/rematrix.c @@ -384,8 +384,10 @@ av_cold static int auto_matrix(SwrContext *s) av_cold int swri_rematrix_init(SwrContext *s){ int i, j; - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : + av_get_channel_layout_nb_channels(s->in_ch_layout); + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : + av_get_channel_layout_nb_channels(s->out_ch_layout); s->mix_any_f = NULL; diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c index d71b41a73e..4f9c92e4ab 100644 --- a/libswresample/x86/rematrix_init.c +++ b/libswresample/x86/rematrix_init.c @@ -33,8 +33,10 @@ D(int16, sse2) av_cold int swri_rematrix_init_x86(struct SwrContext *s){ #if HAVE_X86ASM int mm_flags = av_get_cpu_flags(); - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); + int nb_in = (s->in.ch_count > 0) ? s->in.ch_count : + av_get_channel_layout_nb_channels(s->in_ch_layout); + int nb_out = (s->out.ch_count > 0) ? s->out.ch_count : + av_get_channel_layout_nb_channels(s->out_ch_layout); int num = nb_in * nb_out; int i,j;