Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
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
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
>
--
Marcin Gorzel | Software Engineer | gor...@google.com |
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
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
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No snowflake in an avalanche ever feels responsible. -- Voltaire
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
Hi Tobias,
Sounds good, thanks a lot!
Regards,
Marcin
On Thu, Jul 19, 2018 at 12:53 PM 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".
>
> 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
>
>
--
Marcin Gorzel | Software Engineer | gor...@google.com |
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
Thanks for your input Tobias!
On Wed, Jul 18, 2018 at 3:23 PM Tobias Rapp wrote:
> On 13.07.2018 13:43, 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.
> >
> > This patch adds the following check to the swri_rematrix_init() in
> rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36: if
> channel layout is non-zero, obtain channel count from the channel layout,
> otherwise, use channel count instead.
> >
> > It also modifies the checks in swr_set_matrix() in rematrix.c:72 to
> match the above checks. (Since
> av_get_channel_layout_nb_channels(s->user_in_ch_layout) was originally
> used, there may be preference to rely on the channel layout first (if
> available) before falling back to the user channel count).
> > ---
> > libswresample/rematrix.c | 18 --
> > libswresample/x86/rematrix_init.c | 8 ++--
> > 2 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c
> > index 8227730056..8c9fbf3804 100644
> > --- a/libswresample/rematrix.c
> > +++ b/libswresample/rematrix.c
> > @@ -69,10 +69,12 @@ int swr_set_matrix(struct SwrContext *s, const
> double *matrix, int stride)
> > return AVERROR(EINVAL);
> > memset(s->matrix, 0, sizeof(s->matrix));
> > memset(s->matrix_flt, 0, sizeof(s->matrix_flt));
> > -nb_in = (s->user_in_ch_count > 0) ? s->user_in_ch_count :
> > -av_get_channel_layout_nb_channels(s->user_in_ch_layout);
> > -nb_out = (s->user_out_ch_count > 0) ? s->user_out_ch_count :
> > -av_get_channel_layout_nb_channels(s->user_out_ch_layout);
> > +nb_in = s->user_in_ch_layout != 0
> > +? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
> > +: s->user_in_ch_count;
> > +nb_out = s->user_out_ch_layout != 0
> > +? av_get_channel_layout_nb_channels(s->user_out_ch_layout)
> > +: s->user_out_ch_count;
> > for (out = 0; out < nb_out; out++) {
> > for (in = 0; in < nb_in; in++)
> > s->matrix_flt[out][in] = s->matrix[out][in] = matrix[in];
>
> Sorry for jumping into the discussion late but I don't think this change
> is necessary. If only one of the user_*_ch_count / user_*_ch_layout
> field pair is set, the outcome will be the same. If both field values
> are set they must be consistent or undefined behavior will occur. So if
> we assume the field values are consistent, why use the value that has to
> be calculated first from the layout mask instead of the explicit count
> value?
>
Makes sense. I am new to this code and I wasn't sure what historical
reasons were behind using av_get_channel_layout_nb_channels(layout) to get
the channel count in the first place so I thought it would be safer to
leave the code so that it is used first, before falling back to the channel
count.
> @@ -384,8 +386,12 @@ 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_layout != 0
> > +? av_get_channel_layout_nb_channels(s->in_ch_layout)
> > +: s->user_in_ch_count;
> > +int nb_out = s->out_ch_layout != 0
> > +? av_get_channel_layout_nb_channels(s->out_ch_layout)
> > +: s->user_out_ch_count;
> >
> > s->mix_any_f = NULL;
> >
> > diff --git a/libswresample/x86/rematrix_init.c
> b/libswresample/x86/rematrix_init.c
> > index d71b41a73e..a6ae074926 100644
> > --- a/libswresample/x86/rematrix_init.c
> > +++ b/libswresample/x86/rematrix_init.c
> > @@ -33,8 +33,12 @@ 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_layout != 0
> > +? av_get_channel_layout_nb_channels(s->in_ch_layout)
> > +: s->user_in_ch_count;
> > +int nb_out = s->out_ch_layout != 0
> > +? av_get_channel_layout_nb_channels(s->out_ch_layout)
> > +: s->user_out_ch_count;
> > int num= nb_in * nb_out;
> > int i,j;
> >
> >
>
> Like said above I think the same effect can be achieved with less CPU
> cycles by using a "(count > 0) ? count :
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
On 13.07.2018 13:43, 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.
This patch adds the following check to the swri_rematrix_init() in
rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36: if channel
layout is non-zero, obtain channel count from the channel layout, otherwise,
use channel count instead.
It also modifies the checks in swr_set_matrix() in rematrix.c:72 to match the
above checks. (Since av_get_channel_layout_nb_channels(s->user_in_ch_layout)
was originally used, there may be preference to rely on the channel layout first
(if available) before falling back to the user channel count).
---
libswresample/rematrix.c | 18 --
libswresample/x86/rematrix_init.c | 8 ++--
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c
index 8227730056..8c9fbf3804 100644
--- a/libswresample/rematrix.c
+++ b/libswresample/rematrix.c
@@ -69,10 +69,12 @@ int swr_set_matrix(struct SwrContext *s, const double
*matrix, int stride)
return AVERROR(EINVAL);
memset(s->matrix, 0, sizeof(s->matrix));
memset(s->matrix_flt, 0, sizeof(s->matrix_flt));
-nb_in = (s->user_in_ch_count > 0) ? s->user_in_ch_count :
-av_get_channel_layout_nb_channels(s->user_in_ch_layout);
-nb_out = (s->user_out_ch_count > 0) ? s->user_out_ch_count :
-av_get_channel_layout_nb_channels(s->user_out_ch_layout);
+nb_in = s->user_in_ch_layout != 0
+? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
+: s->user_in_ch_count;
+nb_out = s->user_out_ch_layout != 0
+? av_get_channel_layout_nb_channels(s->user_out_ch_layout)
+: s->user_out_ch_count;
for (out = 0; out < nb_out; out++) {
for (in = 0; in < nb_in; in++)
s->matrix_flt[out][in] = s->matrix[out][in] = matrix[in];
Sorry for jumping into the discussion late but I don't think this change
is necessary. If only one of the user_*_ch_count / user_*_ch_layout
field pair is set, the outcome will be the same. If both field values
are set they must be consistent or undefined behavior will occur. So if
we assume the field values are consistent, why use the value that has to
be calculated first from the layout mask instead of the explicit count
value?
@@ -384,8 +386,12 @@ 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_layout != 0
+? av_get_channel_layout_nb_channels(s->in_ch_layout)
+: s->user_in_ch_count;
+int nb_out = s->out_ch_layout != 0
+? av_get_channel_layout_nb_channels(s->out_ch_layout)
+: s->user_out_ch_count;
s->mix_any_f = NULL;
diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c
index d71b41a73e..a6ae074926 100644
--- a/libswresample/x86/rematrix_init.c
+++ b/libswresample/x86/rematrix_init.c
@@ -33,8 +33,12 @@ 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_layout != 0
+? av_get_channel_layout_nb_channels(s->in_ch_layout)
+: s->user_in_ch_count;
+int nb_out = s->out_ch_layout != 0
+? av_get_channel_layout_nb_channels(s->out_ch_layout)
+: s->user_out_ch_count;
int num= nb_in * nb_out;
int i,j;
Like said above I think the same effect can be achieved with less CPU
cycles by using a "(count > 0) ? count :
av_get_channel_layout_nb_channels(layout)" code pattern.
Also not sure what is the difference between the "in_ch_layout" field
used here and the "user_in_ch_layout" field used in function swr_set_matrix.
Minor nit: In my personal opinion putting parentheses around the
condition part of the ternary operator would improve readability.
Regards,
Tobias
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
Hi Michael,
Of course we should not check in a change that is incorrect. So thank you
for your thorough review.
However, could you help me understand which part exactly of the below do
you think is incorrect?
Before:
int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout);
This is clearly incorrect, since when s->in_ch_layout = 0, nb_in will be 0,
even if the audio has a valid number of channels (<64). So this is the bug
I'm trying to address.
After my change:
int nb_in = s->in_ch_layout != 0
? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
: s->user_in_ch_count;
In words, "check if the channel layout is valid first. If so, use it to
determine the channel count. If not, use the input channel count".
Could you point out which line of the above is incorrect? Are you
suggesting that this check is unnecessary in general and that we should just
use the s->user_in_ch_count instead?
What was the reason the av_get_channel_layout_nb_channels(s->user_in_ch_layout)
is used here in the first place?
Also, could you help me understand, why this (approved and merged) change
is more correct (
https://github.com/FFmpeg/FFmpeg/commit/6325bd3717348615adafb52e4da2fd01a3007d0a#diff-5e2d16895082a305b95d127ece942c03
)?
Before:
nb_in = av_get_channel_layout_nb_channels(s->user_in_ch_layout);
After:
nb_in = (s->user_in_ch_count > 0)
? s->user_in_ch_count
: av_get_channel_layout_nb_channels(s->user_in_ch_layout);
Even if the s->user_in_ch_count fails the check, how is it assured
that the s->user_in_ch_layout
will be valid?
Thanks and apologies if there is some fundamental misunderstanding on my
side - as I said I am new to ffmpeg code but I would love to improve it by
eliminating the problem I've experienced.
Marcin
On Mon, Jul 16, 2018 at 9:23 PM Michael Niedermayer
wrote:
> On Mon, Jul 16, 2018 at 03:37:15PM +0100, Marcin Gorzel wrote:
> > Hi Michael,
> >
> > On Sat, Jul 14, 2018 at 4:01 PM Michael Niedermayer
>
> > wrote:
> >
> > > On Fri, Jul 13, 2018 at 12:43:36PM +0100, 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.
> > > >
> > > > This patch adds the following check to the swri_rematrix_init() in
> > > rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36: if
> > > channel layout is non-zero, obtain channel count from the channel
> layout,
> > > otherwise, use channel count instead.
> > > >
> > > > It also modifies the checks in swr_set_matrix() in rematrix.c:72 to
> > > match the above checks. (Since
> > > av_get_channel_layout_nb_channels(s->user_in_ch_layout) was originally
> > > used, there may be preference to rely on the channel layout first (if
> > > available) before falling back to the user channel count).
> > > > ---
> > > > libswresample/rematrix.c | 18 --
> > > > libswresample/x86/rematrix_init.c | 8 ++--
> > > > 2 files changed, 18 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c
> > > > index 8227730056..8c9fbf3804 100644
> > > > --- a/libswresample/rematrix.c
> > > > +++ b/libswresample/rematrix.c
> > > > @@ -69,10 +69,12 @@ int swr_set_matrix(struct SwrContext *s, const
> > > double *matrix, int stride)
> > > > return AVERROR(EINVAL);
> > > > memset(s->matrix, 0, sizeof(s->matrix));
> > > > memset(s->matrix_flt, 0, sizeof(s->matrix_flt));
> > > > -nb_in = (s->user_in_ch_count > 0) ? s->user_in_ch_count :
> > > > -av_get_channel_layout_nb_channels(s->user_in_ch_layout);
> > > > -nb_out = (s->user_out_ch_count > 0) ? s->user_out_ch_count :
> > > > -av_get_channel_layout_nb_channels(s->user_out_ch_layout);
> > > > +nb_in = s->user_in_ch_layout != 0
> > > > +? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
> > > > +: s->user_in_ch_count;
> > > > +nb_out = s->user_out_ch_layout != 0
> > > > +? av_get_channel_layout_nb_channels(s->user_out_ch_layout)
> > > > +: s->user_out_ch_count;
> > > > for (out = 0; out < nb_out; out++) {
> > > > for (in = 0; in < nb_in; in++)
> > > > s->matrix_flt[out][in] = s->matrix[out][in] =
> matrix[in];
> > >
> > > > @@ -384,8 +386,12 @@ av_cold static int auto_matrix(SwrContext *s)
> > > >
> > > > av_cold int swri_rematrix_init(SwrContext *s){
> > > > int i, j;
> > > > -int nb_in =
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
On Mon, Jul 16, 2018 at 03:37:15PM +0100, Marcin Gorzel wrote:
> Hi Michael,
>
> On Sat, Jul 14, 2018 at 4:01 PM Michael Niedermayer
> wrote:
>
> > On Fri, Jul 13, 2018 at 12:43:36PM +0100, 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.
> > >
> > > This patch adds the following check to the swri_rematrix_init() in
> > rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36: if
> > channel layout is non-zero, obtain channel count from the channel layout,
> > otherwise, use channel count instead.
> > >
> > > It also modifies the checks in swr_set_matrix() in rematrix.c:72 to
> > match the above checks. (Since
> > av_get_channel_layout_nb_channels(s->user_in_ch_layout) was originally
> > used, there may be preference to rely on the channel layout first (if
> > available) before falling back to the user channel count).
> > > ---
> > > libswresample/rematrix.c | 18 --
> > > libswresample/x86/rematrix_init.c | 8 ++--
> > > 2 files changed, 18 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c
> > > index 8227730056..8c9fbf3804 100644
> > > --- a/libswresample/rematrix.c
> > > +++ b/libswresample/rematrix.c
> > > @@ -69,10 +69,12 @@ int swr_set_matrix(struct SwrContext *s, const
> > double *matrix, int stride)
> > > return AVERROR(EINVAL);
> > > memset(s->matrix, 0, sizeof(s->matrix));
> > > memset(s->matrix_flt, 0, sizeof(s->matrix_flt));
> > > -nb_in = (s->user_in_ch_count > 0) ? s->user_in_ch_count :
> > > -av_get_channel_layout_nb_channels(s->user_in_ch_layout);
> > > -nb_out = (s->user_out_ch_count > 0) ? s->user_out_ch_count :
> > > -av_get_channel_layout_nb_channels(s->user_out_ch_layout);
> > > +nb_in = s->user_in_ch_layout != 0
> > > +? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
> > > +: s->user_in_ch_count;
> > > +nb_out = s->user_out_ch_layout != 0
> > > +? av_get_channel_layout_nb_channels(s->user_out_ch_layout)
> > > +: s->user_out_ch_count;
> > > for (out = 0; out < nb_out; out++) {
> > > for (in = 0; in < nb_in; in++)
> > > s->matrix_flt[out][in] = s->matrix[out][in] = matrix[in];
> >
> > > @@ -384,8 +386,12 @@ 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_layout != 0
> > > +? av_get_channel_layout_nb_channels(s->in_ch_layout)
> > > +: s->user_in_ch_count;
> > > +int nb_out = s->out_ch_layout != 0
> > > +? av_get_channel_layout_nb_channels(s->out_ch_layout)
> > > +: s->user_out_ch_count;
> >
> > So this is the core of the change (the other hunk is a "duplicate" and one
> > cosmetic)
> >
>
> Correct. I hope my corrected commit message makes it clearer now?
>
>
> >
> > The code after this uses C ? A : B;
> > this implies that A is wrong in some cases and B is wrong in some cases
> > you explained only one of these, that is that the layout is unable to
> > represent some cases.
> >
>
> B can be wrong if the number of channels exceed the max. allowed value
> (currently 64)
If the number of channels exceed the maximum you should not reach this code.
nothing in your context can ever be out of range because
you never would get beyond checking the users parameters. Such check would
fail and nothing would use the parameters after that.
So you should never reach any check that could use an alternative
Which is what i meant, "why do we need a check here"
This is effectivly saying that the channel count (when the correct field is
used) in the context isnt actually containing the channel count.
I hope this makes it more clear why this change doesnt look correct to me.
> in which case the re-matrixing fails with the 'invalid
> parameter' error message.
> As discussed earlier, I can add an error log to the ibavcodec (as a
> separate patch) that will inform more clearly when the number of channels
> is unsupported.
Adding an error message where one is missing is probably a good idea.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
Hi Michael,
On Sat, Jul 14, 2018 at 4:01 PM Michael Niedermayer
wrote:
> On Fri, Jul 13, 2018 at 12:43:36PM +0100, 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.
> >
> > This patch adds the following check to the swri_rematrix_init() in
> rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36: if
> channel layout is non-zero, obtain channel count from the channel layout,
> otherwise, use channel count instead.
> >
> > It also modifies the checks in swr_set_matrix() in rematrix.c:72 to
> match the above checks. (Since
> av_get_channel_layout_nb_channels(s->user_in_ch_layout) was originally
> used, there may be preference to rely on the channel layout first (if
> available) before falling back to the user channel count).
> > ---
> > libswresample/rematrix.c | 18 --
> > libswresample/x86/rematrix_init.c | 8 ++--
> > 2 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c
> > index 8227730056..8c9fbf3804 100644
> > --- a/libswresample/rematrix.c
> > +++ b/libswresample/rematrix.c
> > @@ -69,10 +69,12 @@ int swr_set_matrix(struct SwrContext *s, const
> double *matrix, int stride)
> > return AVERROR(EINVAL);
> > memset(s->matrix, 0, sizeof(s->matrix));
> > memset(s->matrix_flt, 0, sizeof(s->matrix_flt));
> > -nb_in = (s->user_in_ch_count > 0) ? s->user_in_ch_count :
> > -av_get_channel_layout_nb_channels(s->user_in_ch_layout);
> > -nb_out = (s->user_out_ch_count > 0) ? s->user_out_ch_count :
> > -av_get_channel_layout_nb_channels(s->user_out_ch_layout);
> > +nb_in = s->user_in_ch_layout != 0
> > +? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
> > +: s->user_in_ch_count;
> > +nb_out = s->user_out_ch_layout != 0
> > +? av_get_channel_layout_nb_channels(s->user_out_ch_layout)
> > +: s->user_out_ch_count;
> > for (out = 0; out < nb_out; out++) {
> > for (in = 0; in < nb_in; in++)
> > s->matrix_flt[out][in] = s->matrix[out][in] = matrix[in];
>
> > @@ -384,8 +386,12 @@ 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_layout != 0
> > +? av_get_channel_layout_nb_channels(s->in_ch_layout)
> > +: s->user_in_ch_count;
> > +int nb_out = s->out_ch_layout != 0
> > +? av_get_channel_layout_nb_channels(s->out_ch_layout)
> > +: s->user_out_ch_count;
>
> So this is the core of the change (the other hunk is a "duplicate" and one
> cosmetic)
>
Correct. I hope my corrected commit message makes it clearer now?
>
> The code after this uses C ? A : B;
> this implies that A is wrong in some cases and B is wrong in some cases
> you explained only one of these, that is that the layout is unable to
> represent some cases.
>
B can be wrong if the number of channels exceed the max. allowed value
(currently 64) in which case the re-matrixing fails with the 'invalid
parameter' error message.
As discussed earlier, I can add an error log to the ibavcodec (as a
separate patch) that will inform more clearly when the number of channels
is unsupported.
>
> 2nd question is, are these the ideal fields.
> shouldnt this use s->used_ch_count and s->out.ch_count?
>
I believe so:
s->out.ch_count is set from s-> user_out_ch_count anyway (in
swresample.c:167)
Also, I think s->used_ch_count is only used of input channel count.
These fields were also used in the original change here
.
>
> [...]
> --
> 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
>
--
Marcin Gorzel | Software Engineer | gor...@google.com |
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined
On Fri, Jul 13, 2018 at 12:43:36PM +0100, 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.
>
> This patch adds the following check to the swri_rematrix_init() in
> rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36: if channel
> layout is non-zero, obtain channel count from the channel layout, otherwise,
> use channel count instead.
>
> It also modifies the checks in swr_set_matrix() in rematrix.c:72 to match the
> above checks. (Since av_get_channel_layout_nb_channels(s->user_in_ch_layout)
> was originally used, there may be preference to rely on the channel layout
> first (if available) before falling back to the user channel count).
> ---
> libswresample/rematrix.c | 18 --
> libswresample/x86/rematrix_init.c | 8 ++--
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c
> index 8227730056..8c9fbf3804 100644
> --- a/libswresample/rematrix.c
> +++ b/libswresample/rematrix.c
> @@ -69,10 +69,12 @@ int swr_set_matrix(struct SwrContext *s, const double
> *matrix, int stride)
> return AVERROR(EINVAL);
> memset(s->matrix, 0, sizeof(s->matrix));
> memset(s->matrix_flt, 0, sizeof(s->matrix_flt));
> -nb_in = (s->user_in_ch_count > 0) ? s->user_in_ch_count :
> -av_get_channel_layout_nb_channels(s->user_in_ch_layout);
> -nb_out = (s->user_out_ch_count > 0) ? s->user_out_ch_count :
> -av_get_channel_layout_nb_channels(s->user_out_ch_layout);
> +nb_in = s->user_in_ch_layout != 0
> +? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
> +: s->user_in_ch_count;
> +nb_out = s->user_out_ch_layout != 0
> +? av_get_channel_layout_nb_channels(s->user_out_ch_layout)
> +: s->user_out_ch_count;
> for (out = 0; out < nb_out; out++) {
> for (in = 0; in < nb_in; in++)
> s->matrix_flt[out][in] = s->matrix[out][in] = matrix[in];
> @@ -384,8 +386,12 @@ 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_layout != 0
> +? av_get_channel_layout_nb_channels(s->in_ch_layout)
> +: s->user_in_ch_count;
> +int nb_out = s->out_ch_layout != 0
> +? av_get_channel_layout_nb_channels(s->out_ch_layout)
> +: s->user_out_ch_count;
So this is the core of the change (the other hunk is a "duplicate" and one
cosmetic)
The code after this uses C ? A : B;
this implies that A is wrong in some cases and B is wrong in some cases
you explained only one of these, that is that the layout is unable to
represent some cases.
2nd question is, are these the ideal fields.
shouldnt this use s->used_ch_count and s->out.ch_count?
[...]
--
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
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
