[FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined

[Marcin Gorzel]

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 at 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 at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


-- 

Marcin Gorzel |  Software Engineer |  gorzel at google.com |