Message ID | 148ce8f6-476b-a8b1-a7ae-d44cd2e44316@gmail.com |
---|---|
State | New |
Headers | show |
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@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.stream@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 [...]
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<pkv.stream@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 ? 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
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; 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 { int nb_frame_sizes; SpecifierOpt *frame_pix_fmts; int nb_frame_pix_fmts; + SpecifierOpt *channel_layouts; + int nb_channel_layouts; /* input options */ int64_t input_ts_offset; @@ -624,6 +626,7 @@ extern int vstats_version; extern const AVIOInterruptCB int_cb; extern const OptionDef options[]; +extern const OptionDef channel_layout_option[]; extern const HWAccel hwaccels[]; extern AVBufferRef *hw_device_ctx; #if CONFIG_QSV diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index 47d3841..ab614a8 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -1803,6 +1803,7 @@ 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); + MATCH_PER_STREAM_OPT(channel_layouts, ui64, audio_enc->channel_layout, oc, st); MATCH_PER_STREAM_OPT(sample_fmts, str, sample_fmt, oc, st); if (sample_fmt && @@ -2527,7 +2528,11 @@ loop_end: (count + 1) * sizeof(*f->sample_rates)); } if (ost->enc_ctx->channels) { - f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); + if (ost->enc_ctx->channel_layout) { + f->channel_layout = ost->enc_ctx->channel_layout; + } else { + f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); + } } else if (ost->enc->channel_layouts) { count = 0; while (ost->enc->channel_layouts[count]) @@ -3104,8 +3109,9 @@ static int opt_channel_layout(void *optctx, const char *opt, const char *arg) char layout_str[32]; char *stream_str; char *ac_str; - int ret, channels, ac_str_size; + int ret, channels, ac_str_size, stream_str_size; uint64_t layout; + const char *spec = "a"; layout = av_get_channel_layout(arg); if (!layout) { @@ -3116,12 +3122,33 @@ static int opt_channel_layout(void *optctx, const char *opt, const char *arg) ret = opt_default_new(o, opt, layout_str); if (ret < 0) return ret; + /* factored code between 'ac' option and 'channel_layout_spec' option */ + stream_str = strchr(opt, ':'); + stream_str_size = (stream_str ? strlen(stream_str) : 0); + /* set 'channel_layout_spec' internal option which stores channel_layout + * (as uint64) in SpecifierOpt, which enables access to channel_layout through MATCH_PER_STREAM_OPT + */ + GROW_ARRAY(o->channel_layouts, o->nb_channel_layouts); + o->channel_layouts[o->nb_channel_layouts - 1].specifier = spec; + ac_str_size = 20 + stream_str_size; + ac_str = av_mallocz(ac_str_size); + if (!ac_str) { + return AVERROR(ENOMEM); + } + av_strlcpy(ac_str, "channel_layout_spec", 20); + if (stream_str) { + av_strlcat(ac_str, stream_str, ac_str_size); + } + ret = parse_option(o, ac_str, layout_str, channel_layout_option); + if (ret < 0) { + return ret; + } + av_free(ac_str); /* set 'ac' option based on channel layout */ channels = av_get_channel_layout_nb_channels(layout); snprintf(layout_str, sizeof(layout_str), "%d", channels); - stream_str = strchr(opt, ':'); - ac_str_size = 3 + (stream_str ? strlen(stream_str) : 0); + ac_str_size = 3 + stream_str_size; ac_str = av_mallocz(ac_str_size); if (!ac_str) return AVERROR(ENOMEM); @@ -3758,3 +3785,9 @@ const OptionDef options[] = { { NULL, }, }; +/* internal OptionDef used to enable storage of channel_layout option in a SpecifierOpt */ +const OptionDef channel_layout_option[] = { + { "channel_layout_spec", OPT_AUDIO | HAS_ARG | OPT_INT64 | OPT_SPEC | + OPT_INPUT | OPT_OUTPUT,{ .off = OFFSET(channel_layouts) }, + "stores channel layout in SpecifierOpt ", "layout_hex" }, +};