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<pkv.stream at gmail.com>
> >>>>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 at 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 <pkv.stream at gmail.com>
> 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;
> int i;
> int64_t i64;
> + uint64_t ui64;
> float f;
> double dbl;
> } u;
please split this in a seperate patch
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171118/e3d00625/attachment.sig>