Re: [FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

2017-11-18 Thread pkv.stream 

Le 18/11/2017 à 9:26 PM, Michael Niedermayer a écrit :

On Sat, Nov 18, 2017 at 11:41:54AM +0100, pkv.stream wrote:

Hi Michael

  ffmpeg_opt.c |   12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)
a7c71fb1ccd7d91b61033be70dfd324b4e3f3c34  
0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch
 From fb7f7f6e01cc242e13d0e640f583dffe6e7a8ada Mon Sep 17 00:00:00 2001
From: pkviet
Date: Mon, 2 Oct 2017 11:14:31 +0200
Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout

Fix for ticket 6706.
The -channel_layout option was not working when the channel layout was not
a default one (ex: for 4 channels, quad was interpreted as 4.0 which is
the default layout for 4 channels; or octagonal interpreted as 7.1).
This led to the spurious auto-insertion of an auto-resampler filter
remapping the channels even if input and output had identical channel
layouts.
The fix operates by directly calling the channel layout if defined in
options. If the layout is undefined, the default layout is selected as
before the fix.
---
  fftools/ffmpeg_opt.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index ca6f10d..8941d66 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1785,6 +1785,7 @@ static OutputStream *new_audio_stream(OptionsContext *o, 
AVFormatContext *oc, in
  AVStream *st;
  OutputStream *ost;
  AVCodecContext *audio_enc;
+AVDictionaryEntry *output_layout;
  ost = new_output_stream(o, oc, AVMEDIA_TYPE_AUDIO, source_index);
  st  = ost->st;
@@ -1799,7 +1800,10 @@ static OutputStream *new_audio_stream(OptionsContext *o, 
AVFormatContext *oc, in
  char *sample_fmt = NULL;
  MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st);
-
+output_layout = av_dict_get(ost->encoder_opts,"channel_layout", NULL, 
AV_DICT_IGNORE_SUFFIX);
+if (output_layout) {
+audio_enc->channel_layout = strtoull(output_layout->value, NULL, 
10);
+}

why is this handled different from audio_channels ?
that is why is this not using MATCH_PER_STREAM_OPT() ?
(yes this would require some changes but i wonder why this would be
  handled differently)

Hi
I did try to use the MATCH_PER_STREAM_OPT() but didn't manage to
have it working. Also I was a bit hesitant to modify the
OptionsContext struct, and preferred something minimal.
If you think this can definitely be made to work without too much
coding and prefer such a solution, I'll retry working on a
MATCH_PER_STREAM_OPT() solution.

i dont really know if it "can definitely be made to work without too much
coding", it just seemed odd how this is handled differently

I have another version of the patch working with MATCH_PER_STREAM_OPT() ;
but the changes to code are more important, and it's a bit hacky
(defines an internal OptionDef) ... so not sure it is any better
than the few lines of patch v3.
I've checked that stream specifiers ( :a:0 ) are passed correctly
and that two streams with different layouts are also treated
correctly (the previous patch without MATCH_PER_STREAM_OPT() works
also; those were two points you singled out in your review).
  I have no real opinion on the best course, which to pick or even to
let the bug hanging (I'm only trying to fix it due to my work on the
aac PCE patch of atomnuker ; the bug prevents full use of the new
PCE capability).
It's Ok with me if you decide to forgo these attempts to fix the bug
and let the bug stay.
I'm not impacted by the bug in my case use (encode 16 channels in
aac); just trying to be thorough in my (akward) contributions and
trying to give back to the project.
Tell me the best course; or if you see a way to make my
MATCH_PER_STREAM_OPT() code less hacky.

iam sure theres a way to do this less hacky
why do you need a 2nd table ? or rather why does it not work if you
put the entry in the main table ? (so there are 2 entries one for
OPT_SPEC and one for teh callback, will it not send the data to both
matching entries ?


Hi
it does work in the main table.
I didn't want to pollute it. But if you say it is OK then I'll update 
accordingly and send a separate patch then.

thanks

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

2017-11-18 Thread Michael Niedermayer 
On Sat, Nov 18, 2017 at 11:41:54AM +0100, pkv.stream wrote:
> Hi Michael
>   ffmpeg_opt.c |   12 ++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> a7c71fb1ccd7d91b61033be70dfd324b4e3f3c34  
> 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch
>  From fb7f7f6e01cc242e13d0e640f583dffe6e7a8ada Mon Sep 17 00:00:00 2001
> From: pkviet
> Date: Mon, 2 Oct 2017 11:14:31 +0200
> Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout
> 
> Fix for ticket 6706.
> The -channel_layout option was not working when the channel layout was not
> a default one (ex: for 4 channels, quad was interpreted as 4.0 which is
> the default layout for 4 channels; or octagonal interpreted as 7.1).
> This led to the spurious auto-insertion of an auto-resampler filter
> remapping the channels even if input and output had identical channel
> layouts.
> The fix operates by directly calling the channel layout if defined in
> options. If the layout is undefined, the default layout is selected as
> before the fix.
> ---
>   fftools/ffmpeg_opt.c | 12 ++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index ca6f10d..8941d66 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -1785,6 +1785,7 @@ static OutputStream 
> *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in
>   AVStream *st;
>   OutputStream *ost;
>   AVCodecContext *audio_enc;
> +AVDictionaryEntry *output_layout;
>   ost = new_output_stream(o, oc, AVMEDIA_TYPE_AUDIO, source_index);
>   st  = ost->st;
> @@ -1799,7 +1800,10 @@ static OutputStream 
> *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in
>   char *sample_fmt = NULL;
>   MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, 
>  oc, st);
> -
> +output_layout = av_dict_get(ost->encoder_opts,"channel_layout", 
> NULL, AV_DICT_IGNORE_SUFFIX);
> +if (output_layout) {
> +audio_enc->channel_layout = strtoull(output_layout->value, 
> NULL, 10);
> +}
> >>>why is this handled different from audio_channels ?
> >>>that is why is this not using MATCH_PER_STREAM_OPT() ?
> >>>(yes this would require some changes but i wonder why this would be
> >>>  handled differently)
> >>Hi
> >>I did try to use the MATCH_PER_STREAM_OPT() but didn't manage to
> >>have it working. Also I was a bit hesitant to modify the
> >>OptionsContext struct, and preferred something minimal.
> >>If you think this can definitely be made to work without too much
> >>coding and prefer such a solution, I'll retry working on a
> >>MATCH_PER_STREAM_OPT() solution.
> >i dont really know if it "can definitely be made to work without too much
> >coding", it just seemed odd how this is handled differently
> 
> I have another version of the patch working with MATCH_PER_STREAM_OPT() ;
> but the changes to code are more important, and it's a bit hacky
> (defines an internal OptionDef) ... so not sure it is any better
> than the few lines of patch v3.
> I've checked that stream specifiers ( :a:0 ) are passed correctly
> and that two streams with different layouts are also treated
> correctly (the previous patch without MATCH_PER_STREAM_OPT() works
> also; those were two points you singled out in your review).
>  I have no real opinion on the best course, which to pick or even to
> let the bug hanging (I'm only trying to fix it due to my work on the
> aac PCE patch of atomnuker ; the bug prevents full use of the new
> PCE capability).
> It's Ok with me if you decide to forgo these attempts to fix the bug
> and let the bug stay.
> I'm not impacted by the bug in my case use (encode 16 channels in
> aac); just trying to be thorough in my (akward) contributions and
> trying to give back to the project.

> Tell me the best course; or if you see a way to make my
> MATCH_PER_STREAM_OPT() code less hacky.

iam sure theres a way to do this less hacky
why do you need a 2nd table ? or rather why does it not work if you
put the entry in the main table ? (so there are 2 entries one for
OPT_SPEC and one for teh callback, will it not send the data to both
matching entries ?



> 
> Regards
> 
> >[...]
> >
> >
> >___
> >ffmpeg-devel mailing list
> >ffmpeg-devel@ffmpeg.org
> >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 

>  cmdutils.h   |1 +
>  ffmpeg.h |3 +++
>  ffmpeg_opt.c |   41 +
>  3 files changed, 41 insertions(+), 4 deletions(-)
> 7c1249f0cb4daa1aebbf94b0e785e644997f754a  0001-ffmpeg-fix-ticket-6706.patch
> From 00c3c724544b16c19282b39644e2584f9c4a4181 Mon Sep 17 00:00:00 2001
> From: pkviet 
> Date: Sat, 18 Nov 2017

Re: [FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

2017-11-18 Thread pkv.stream 

Hi Michael

  ffmpeg_opt.c |   12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)
a7c71fb1ccd7d91b61033be70dfd324b4e3f3c34  
0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch
 From fb7f7f6e01cc242e13d0e640f583dffe6e7a8ada Mon Sep 17 00:00:00 2001
From: pkviet
Date: Mon, 2 Oct 2017 11:14:31 +0200
Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout

Fix for ticket 6706.
The -channel_layout option was not working when the channel layout was not
a default one (ex: for 4 channels, quad was interpreted as 4.0 which is
the default layout for 4 channels; or octagonal interpreted as 7.1).
This led to the spurious auto-insertion of an auto-resampler filter
remapping the channels even if input and output had identical channel
layouts.
The fix operates by directly calling the channel layout if defined in
options. If the layout is undefined, the default layout is selected as
before the fix.
---
  fftools/ffmpeg_opt.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index ca6f10d..8941d66 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1785,6 +1785,7 @@ static OutputStream *new_audio_stream(OptionsContext *o, 
AVFormatContext *oc, in
  AVStream *st;
  OutputStream *ost;
  AVCodecContext *audio_enc;
+AVDictionaryEntry *output_layout;
  ost = new_output_stream(o, oc, AVMEDIA_TYPE_AUDIO, source_index);
  st  = ost->st;
@@ -1799,7 +1800,10 @@ static OutputStream *new_audio_stream(OptionsContext *o, 
AVFormatContext *oc, in
  char *sample_fmt = NULL;
  MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st);
-
+output_layout = av_dict_get(ost->encoder_opts,"channel_layout", NULL, 
AV_DICT_IGNORE_SUFFIX);
+if (output_layout) {
+audio_enc->channel_layout = strtoull(output_layout->value, NULL, 
10);
+}

why is this handled different from audio_channels ?
that is why is this not using MATCH_PER_STREAM_OPT() ?
(yes this would require some changes but i wonder why this would be
  handled differently)

Hi
I did try to use the MATCH_PER_STREAM_OPT() but didn't manage to
have it working. Also I was a bit hesitant to modify the
OptionsContext struct, and preferred something minimal.
If you think this can definitely be made to work without too much
coding and prefer such a solution, I'll retry working on a
MATCH_PER_STREAM_OPT() solution.

i dont really know if it "can definitely be made to work without too much
coding", it just seemed odd how this is handled differently


I have another version of the patch working with MATCH_PER_STREAM_OPT() ;
but the changes to code are more important, and it's a bit hacky 
(defines an internal OptionDef) ... so not sure it is any better than 
the few lines of patch v3.
I've checked that stream specifiers ( :a:0 ) are passed correctly and 
that two streams with different layouts are also treated correctly (the 
previous patch without MATCH_PER_STREAM_OPT() works also; those were two 
points you singled out in your review).
 I have no real opinion on the best course, which to pick or even to 
let the bug hanging (I'm only trying to fix it due to my work on the aac 
PCE patch of atomnuker ; the bug prevents full use of the new PCE 
capability).
It's Ok with me if you decide to forgo these attempts to fix the bug and 
let the bug stay.
I'm not impacted by the bug in my case use (encode 16 channels in aac); 
just trying to be thorough in my (akward) contributions and trying to 
give back to the project.
Tell me the best course; or if you see a way to make my 
MATCH_PER_STREAM_OPT() code less hacky.


Regards


[...]


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



From 00c3c724544b16c19282b39644e2584f9c4a4181 Mon Sep 17 00:00:00 2001
From: pkviet 
Date: Sat, 18 Nov 2017 00:26:50 +0100
Subject: [PATCH] ffmpeg: fix ticket 6706

Fix regression with channel_layout option which is not passed
correctly from output streams to filters when the channel layout is not
a default one.
---
 fftools/cmdutils.h   |  1 +
 fftools/ffmpeg.h |  3 +++
 fftools/ffmpeg_opt.c | 41 +
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
index 2997ee3..fa2b255 100644
--- a/fftools/cmdutils.h
+++ b/fftools/cmdutils.h
@@ -155,6 +155,7 @@ typedef struct SpecifierOpt {
 uint8_t *str;
 inti;
 int64_t  i64;
+uint64_t ui64;
 float  f;
 double   dbl;
 } u;
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
index e0977e1..5c45df4 100644
--- a/fftools/ffmpeg.h
+++ b/fftools/ffmpeg.h
@@ -121,6 +121,8 @@ typedef struct OptionsContext {
 intnb_frame_sizes;
 SpecifierOpt *frame_pix_fmts;
 int

Re: [FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

2017-11-15 Thread Michael Niedermayer 
On Tue, Nov 14, 2017 at 11:49:26PM +0100, pkv.stream wrote:
> Le 14/11/2017 à 1:13 PM, Michael Niedermayer a écrit :
> >On Sun, Nov 12, 2017 at 06:26:18PM +0100, pkv.stream wrote:
> >>Le 12/11/2017 à 5:38 PM, Michael Niedermayer a écrit :
> >>>On Sun, Nov 12, 2017 at 05:07:04PM +0100, Kv Pham wrote:
> Le 12 nov. 2017 5:01 PM, "Michael Niedermayer"  a
> écrit :
> 
> On Sat, Nov 11, 2017 at 02:15:25AM +0100, pkv.stream wrote:
> >Le 11/11/2017 à 1:07 AM, Michael Niedermayer a écrit :
> >>On Fri, Nov 10, 2017 at 10:27:48PM +0100, pkv.stream wrote:
> >>>Le 10/11/2017 à 1:12 AM, Michael Niedermayer a écrit :
> On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote:
> >Hi Michael,
> >
> >>>  ffmpeg_opt.c |   11 ++-
> >>>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>>2af07f4366efdfaf1018bb2ea29be672befe0823  0001-ffmpeg-fix-channel_
> layout-bug-on-non-default-layout.patch
> >>> From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00
> 2001
> >>>From: pkviet
> >>>Date: Mon, 2 Oct 2017 11:14:31 +0200
> >>>Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default
> layout
> >>>Fix for ticket 6706.
> >>>The -channel_layout option was not working when the channel layout
> was not
> >>>a default one (ex: for 4 channels, quad was interpreted as 4.0
> which is
> >>>the default layout for 4 channels; or octagonal interpreted as 
> >>>7.1).
> >>>This led to the spurious auto-insertion of an auto-resampler filter
> >>>remapping the channels even if input and output had identical
> channel
> >>>layouts.
> >>>The fix operates by directly calling the channel layout if defined
> in
> >>>options. If the layout is undefined, the default layout is selected
> as
> >>>before the fix.
> >>>---
> >>>  ffmpeg_opt.c | 11 ++-
> >>>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> >>>index 100fa76..cf5a63c 100644
> >>>--- a/ffmpeg_opt.c
> >>>+++ b/ffmpeg_opt.c
> >>>@@ -1804,6 +1804,12 @@ static OutputStream 
> >>>*new_audio_stream(OptionsContext
> *o, AVFormatContext *oc, in
> >>>  MATCH_PER_STREAM_OPT(audio_channels, i,
> audio_enc->channels, oc, st);
> >>>+AVDictionaryEntry *output_layout =
> av_dict_get(o->g->codec_opts,
> >>>+
>   "channel_layout",
> >>>+   NULL,
> AV_DICT_MATCH_CASE);
> >>This doesnt look right
> >>
> >>not an issue of the patch as such but
> >>why is the channel_layout option per file and not per stream?
> >just my ignorance; do you mean this is not the right way to retrieve
> >the channel_layout option from an audio stream ?
> I think there is more buggy with how the channel_layout is handled
> 
> try this:
> ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav
> and this:
> ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 
> test.wav
> 
> while it may appear that the are both working this is deceiving.
> I think only the channel number gets actually used in the 2nd case
> 
> Look at what your code with av_dict_get() reads.
> It does not obtain the 5 in the 2nd case
> >>>ok I fixed that by using the flag AV_DICT_IGNORE_SUFFIX ; now
> >>>-channel_layout:a works as expected.
> >>>I fixed also all the styling issues you spotted (mixed declarations,
> >>>{} for if etc).
> >>>
> >>>Still not understanding your initial comment though :
> >>>'why is the channel_layout option per file and not per stream?'
> >>>
> >>>do you mean o->g->codec_opts is not where I should retrieve the
> >>>channel_layout ? if not where from ?
> >>>
> >>>Thanks
> >>>
> [...]
> 
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>  ffmpeg_opt.c |   12 ++--
> >>>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>>849898d28e989ffa5cc598c6fe4d43847b636132  0001-ffmpeg-fix-channel_
> layout-bug-on-non-default-layout.patch
> >>> From 6d4f1c14135f9473b77e1c649d0e7bbaeba00a50 Mon Sep 17 00:00:00 2001
> >>>From: pkviet
> >>>Date: Mon, 2 Oct 2017 11:14:31 +0200
> >>>Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout
> >>>
> >>>Fix for ticket 6706.
> >>>The -channel_layout option was not working when the

Re: [FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

2017-11-14 Thread pkv.stream 

Le 14/11/2017 à 1:13 PM, Michael Niedermayer a écrit :

On Sun, Nov 12, 2017 at 06:26:18PM +0100, pkv.stream wrote:

Le 12/11/2017 à 5:38 PM, Michael Niedermayer a écrit :

On Sun, Nov 12, 2017 at 05:07:04PM +0100, Kv Pham wrote:

Le 12 nov. 2017 5:01 PM, "Michael Niedermayer"  a
écrit :

On Sat, Nov 11, 2017 at 02:15:25AM +0100, pkv.stream wrote:

Le 11/11/2017 à 1:07 AM, Michael Niedermayer a écrit :

On Fri, Nov 10, 2017 at 10:27:48PM +0100, pkv.stream wrote:

Le 10/11/2017 à 1:12 AM, Michael Niedermayer a écrit :

On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote:

Hi Michael,


  ffmpeg_opt.c |   11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)
2af07f4366efdfaf1018bb2ea29be672befe0823  0001-ffmpeg-fix-channel_

layout-bug-on-non-default-layout.patch

 From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00

2001

From: pkviet
Date: Mon, 2 Oct 2017 11:14:31 +0200
Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default

layout

Fix for ticket 6706.
The -channel_layout option was not working when the channel layout

was not

a default one (ex: for 4 channels, quad was interpreted as 4.0

which is

the default layout for 4 channels; or octagonal interpreted as 7.1).
This led to the spurious auto-insertion of an auto-resampler filter
remapping the channels even if input and output had identical

channel

layouts.
The fix operates by directly calling the channel layout if defined

in

options. If the layout is undefined, the default layout is selected

as

before the fix.
---
  ffmpeg_opt.c | 11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 100fa76..cf5a63c 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext

*o, AVFormatContext *oc, in

  MATCH_PER_STREAM_OPT(audio_channels, i,

audio_enc->channels, oc, st);

+AVDictionaryEntry *output_layout =

av_dict_get(o->g->codec_opts,

+

  "channel_layout",

+   NULL,

AV_DICT_MATCH_CASE);

This doesnt look right

not an issue of the patch as such but
why is the channel_layout option per file and not per stream?

just my ignorance; do you mean this is not the right way to retrieve
the channel_layout option from an audio stream ?

I think there is more buggy with how the channel_layout is handled

try this:
./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav
and this:
./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 test.wav

while it may appear that the are both working this is deceiving.
I think only the channel number gets actually used in the 2nd case

Look at what your code with av_dict_get() reads.
It does not obtain the 5 in the 2nd case

ok I fixed that by using the flag AV_DICT_IGNORE_SUFFIX ; now
-channel_layout:a works as expected.
I fixed also all the styling issues you spotted (mixed declarations,
{} for if etc).

Still not understanding your initial comment though :
'why is the channel_layout option per file and not per stream?'

do you mean o->g->codec_opts is not where I should retrieve the
channel_layout ? if not where from ?

Thanks


[...]


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

  ffmpeg_opt.c |   12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)
849898d28e989ffa5cc598c6fe4d43847b636132  0001-ffmpeg-fix-channel_

layout-bug-on-non-default-layout.patch

 From 6d4f1c14135f9473b77e1c649d0e7bbaeba00a50 Mon Sep 17 00:00:00 2001
From: pkviet
Date: Mon, 2 Oct 2017 11:14:31 +0200
Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout

Fix for ticket 6706.
The -channel_layout option was not working when the channel layout was

not

a default one (ex: for 4 channels, quad was interpreted as 4.0 which is
the default layout for 4 channels; or octagonal interpreted as 7.1).
This led to the spurious auto-insertion of an auto-resampler filter
remapping the channels even if input and output had identical channel
layouts.
The fix operates by directly calling the channel layout if defined in
options. If the layout is undefined, the default layout is selected as
before the fix.
---
  fftools/ffmpeg_opt.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index ca6f10d..cb25d7b 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1785,6 +1785,7 @@ static OutputStream *new_audio_stream(OptionsContext

*o, AVFormatContext *oc, in

  AVStream *st;
  OutputStream *ost;
  AVCodecContext *audio_enc;
+AVDictionaryEntry *output_layout;
  ost = new_output_stream(o, oc, AVMEDIA_TYPE_AUDIO, source_index);
  st  = ost->st;
@@ -1799,7 +1800,10 @@ static OutputStream

Re: [FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

2017-11-14 Thread Michael Niedermayer 
On Sun, Nov 12, 2017 at 06:26:18PM +0100, pkv.stream wrote:
> Le 12/11/2017 à 5:38 PM, Michael Niedermayer a écrit :
> >On Sun, Nov 12, 2017 at 05:07:04PM +0100, Kv Pham wrote:
> >>Le 12 nov. 2017 5:01 PM, "Michael Niedermayer"  a
> >>écrit :
> >>
> >>On Sat, Nov 11, 2017 at 02:15:25AM +0100, pkv.stream wrote:
> >>>Le 11/11/2017 à 1:07 AM, Michael Niedermayer a écrit :
> On Fri, Nov 10, 2017 at 10:27:48PM +0100, pkv.stream wrote:
> >Le 10/11/2017 à 1:12 AM, Michael Niedermayer a écrit :
> >>On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote:
> >>>Hi Michael,
> >>>
> >  ffmpeg_opt.c |   11 ++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >2af07f4366efdfaf1018bb2ea29be672befe0823  0001-ffmpeg-fix-channel_
> >>layout-bug-on-non-default-layout.patch
> > From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00
> >>2001
> >From: pkviet 
> >Date: Mon, 2 Oct 2017 11:14:31 +0200
> >Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default
> >>layout
> >Fix for ticket 6706.
> >The -channel_layout option was not working when the channel layout
> >>was not
> >a default one (ex: for 4 channels, quad was interpreted as 4.0
> >>which is
> >the default layout for 4 channels; or octagonal interpreted as 7.1).
> >This led to the spurious auto-insertion of an auto-resampler filter
> >remapping the channels even if input and output had identical
> >>channel
> >layouts.
> >The fix operates by directly calling the channel layout if defined
> >>in
> >options. If the layout is undefined, the default layout is selected
> >>as
> >before the fix.
> >---
> >  ffmpeg_opt.c | 11 ++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> >diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> >index 100fa76..cf5a63c 100644
> >--- a/ffmpeg_opt.c
> >+++ b/ffmpeg_opt.c
> >@@ -1804,6 +1804,12 @@ static OutputStream 
> >*new_audio_stream(OptionsContext
> >>*o, AVFormatContext *oc, in
> >  MATCH_PER_STREAM_OPT(audio_channels, i,
> >>audio_enc->channels, oc, st);
> >+AVDictionaryEntry *output_layout =
> >>av_dict_get(o->g->codec_opts,
> >+
> >>  "channel_layout",
> >+   NULL,
> >>AV_DICT_MATCH_CASE);
> This doesnt look right
> 
> not an issue of the patch as such but
> why is the channel_layout option per file and not per stream?
> >>>just my ignorance; do you mean this is not the right way to retrieve
> >>>the channel_layout option from an audio stream ?
> >>I think there is more buggy with how the channel_layout is handled
> >>
> >>try this:
> >>./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav
> >>and this:
> >>./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 test.wav
> >>
> >>while it may appear that the are both working this is deceiving.
> >>I think only the channel number gets actually used in the 2nd case
> >>
> >>Look at what your code with av_dict_get() reads.
> >>It does not obtain the 5 in the 2nd case
> >ok I fixed that by using the flag AV_DICT_IGNORE_SUFFIX ; now
> >-channel_layout:a works as expected.
> >I fixed also all the styling issues you spotted (mixed declarations,
> >{} for if etc).
> >
> >Still not understanding your initial comment though :
> >'why is the channel_layout option per file and not per stream?'
> >
> >do you mean o->g->codec_opts is not where I should retrieve the
> >channel_layout ? if not where from ?
> >
> >Thanks
> >
> >>[...]
> >>
> >>
> >>___
> >>ffmpeg-devel mailing list
> >>ffmpeg-devel@ffmpeg.org
> >>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >  ffmpeg_opt.c |   12 ++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >849898d28e989ffa5cc598c6fe4d43847b636132  0001-ffmpeg-fix-channel_
> >>layout-bug-on-non-default-layout.patch
> > From 6d4f1c14135f9473b77e1c649d0e7bbaeba00a50 Mon Sep 17 00:00:00 2001
> >From: pkviet 
> >Date: Mon, 2 Oct 2017 11:14:31 +0200
> >Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout
> >
> >Fix for ticket 6706.
> >The -channel_layout option was not working when the channel layout was
> >>not
> >a default one (ex: for 4 channels, quad was interpreted as 4.0 which is
> >the default layout for 4 channels; or octagonal interpreted as 7.1).
> >This led to the spurious auto-insertion of an auto-resampler filter
> >remapping the channels even if input and output had identical channel
> >layouts.
>

[FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

2017-11-12 Thread pkv.stream 

Le 12/11/2017 à 5:38 PM, Michael Niedermayer a écrit :

On Sun, Nov 12, 2017 at 05:07:04PM +0100, Kv Pham wrote:

Le 12 nov. 2017 5:01 PM, "Michael Niedermayer"  a
écrit :

On Sat, Nov 11, 2017 at 02:15:25AM +0100, pkv.stream wrote:

Le 11/11/2017 à 1:07 AM, Michael Niedermayer a écrit :

On Fri, Nov 10, 2017 at 10:27:48PM +0100, pkv.stream wrote:

Le 10/11/2017 à 1:12 AM, Michael Niedermayer a écrit :

On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote:

Hi Michael,


  ffmpeg_opt.c |   11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)
2af07f4366efdfaf1018bb2ea29be672befe0823  0001-ffmpeg-fix-channel_

layout-bug-on-non-default-layout.patch

 From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00

2001

From: pkviet 
Date: Mon, 2 Oct 2017 11:14:31 +0200
Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default

layout

Fix for ticket 6706.
The -channel_layout option was not working when the channel layout

was not

a default one (ex: for 4 channels, quad was interpreted as 4.0

which is

the default layout for 4 channels; or octagonal interpreted as 7.1).
This led to the spurious auto-insertion of an auto-resampler filter
remapping the channels even if input and output had identical

channel

layouts.
The fix operates by directly calling the channel layout if defined

in

options. If the layout is undefined, the default layout is selected

as

before the fix.
---
  ffmpeg_opt.c | 11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 100fa76..cf5a63c 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext

*o, AVFormatContext *oc, in

  MATCH_PER_STREAM_OPT(audio_channels, i,

audio_enc->channels, oc, st);

+AVDictionaryEntry *output_layout =

av_dict_get(o->g->codec_opts,

+

  "channel_layout",

+   NULL,

AV_DICT_MATCH_CASE);

This doesnt look right

not an issue of the patch as such but
why is the channel_layout option per file and not per stream?

just my ignorance; do you mean this is not the right way to retrieve
the channel_layout option from an audio stream ?

I think there is more buggy with how the channel_layout is handled

try this:
./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav
and this:
./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 test.wav

while it may appear that the are both working this is deceiving.
I think only the channel number gets actually used in the 2nd case

Look at what your code with av_dict_get() reads.
It does not obtain the 5 in the 2nd case

ok I fixed that by using the flag AV_DICT_IGNORE_SUFFIX ; now
-channel_layout:a works as expected.
I fixed also all the styling issues you spotted (mixed declarations,
{} for if etc).

Still not understanding your initial comment though :
'why is the channel_layout option per file and not per stream?'

do you mean o->g->codec_opts is not where I should retrieve the
channel_layout ? if not where from ?

Thanks


[...]


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

  ffmpeg_opt.c |   12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)
849898d28e989ffa5cc598c6fe4d43847b636132  0001-ffmpeg-fix-channel_

layout-bug-on-non-default-layout.patch

 From 6d4f1c14135f9473b77e1c649d0e7bbaeba00a50 Mon Sep 17 00:00:00 2001
From: pkviet 
Date: Mon, 2 Oct 2017 11:14:31 +0200
Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout

Fix for ticket 6706.
The -channel_layout option was not working when the channel layout was

not

a default one (ex: for 4 channels, quad was interpreted as 4.0 which is
the default layout for 4 channels; or octagonal interpreted as 7.1).
This led to the spurious auto-insertion of an auto-resampler filter
remapping the channels even if input and output had identical channel
layouts.
The fix operates by directly calling the channel layout if defined in
options. If the layout is undefined, the default layout is selected as
before the fix.
---
  fftools/ffmpeg_opt.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index ca6f10d..cb25d7b 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1785,6 +1785,7 @@ static OutputStream *new_audio_stream(OptionsContext

*o, AVFormatContext *oc, in

  AVStream *st;
  OutputStream *ost;
  AVCodecContext *audio_enc;
+AVDictionaryEntry *output_layout;
  ost = new_output_stream(o, oc, AVMEDIA_TYPE_AUDIO, source_index);
  st  = ost->st;
@@ -1799,7 +1800,10 @@ static OutputStream *new_audio_stream(OptionsContext

*o, AVFormatContext *oc, in

  char *sample_fmt = NULL;