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.str...@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@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.str...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
