| Message ID | 20180320181108.26163-1-onemda@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 3/20/18, Paul B Mahol <onemda@gmail.com> wrote: > So user can pick which channels to extract. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > doc/filters.texi | 15 ++++++++++++ > libavfilter/af_channelsplit.c | 54 > +++++++++++++++++++++++++++++++++++-------- > 2 files changed, 60 insertions(+), 9 deletions(-) > Please comment, comitting ASAP.
Paul B Mahol (2018-03-20):
> Please comment, comitting ASAP.
Three hours? Really?
Seriously, what is your rush?
Regards,
On 3/20/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-03-20): >> Please comment, comitting ASAP. > > Three hours? Really? > > Seriously, what is your rush? It is snowing, and electricity is near full depletion.
On 3/20/18, Paul B Mahol <onemda@gmail.com> wrote: > On 3/20/18, Nicolas George <george@nsup.org> wrote: >> Paul B Mahol (2018-03-20): >>> Please comment, comitting ASAP. >> >> Three hours? Really? >> >> Seriously, what is your rush? > > It is snowing, and electricity is near full depletion. > Enough hours passed, time to apply.
Paul B Mahol (2018-03-21):
> Enough hours passed, time to apply.
Stop being ridiculous. Wait three days at least, then ping, then wait
one week more if there are no answers.
Regards,
On 3/21/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-03-21): >> Enough hours passed, time to apply. > > Stop being ridiculous. Wait three days at least, then ping, then wait > one week more if there are no answers. I can't wait. You could instead of spending time replying to this mails do review patches. There is not enough reviewers here on this mailing list.
Paul B Mahol (2018-03-21): > I can't wait. Sure you can. Easy: go read wikipedia, go watch youtobe, go run in the neighbourhood, go read a book. Do you need more ideas? > You could instead of spending time replying to this mails > do review patches. Are you under the misapprehension that I am your lackey? > There is not enough reviewers here on this mailing list. Well, too bad. Maybe if you were less rude to people they would not be discouraged. Regards,
On 3/21/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-03-21): >> I can't wait. > > Sure you can. Easy: go read wikipedia, go watch youtobe, go run in the > neighbourhood, go read a book. Do you need more ideas? > >> You could instead of spending time replying to this mails >> do review patches. > > Are you under the misapprehension that I am your lackey? > >> There is not enough reviewers here on this mailing list. > > Well, too bad. Maybe if you were less rude to people they would not be > discouraged. Maybe. But I'm only being rude to 3 people at max.
On Wed, 21 Mar 2018 16:07:32 +0100 Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-03-21): > > I can't wait. > > Sure you can. Easy: go read wikipedia, go watch youtobe, go run in the > neighbourhood, go read a book. Do you need more ideas? > > > You could instead of spending time replying to this mails > > do review patches. > > Are you under the misapprehension that I am your lackey? > > > There is not enough reviewers here on this mailing list. > > Well, too bad. Maybe if you were less rude to people they would not be > discouraged. I don't want to start another thing, but that sure is rich coming from you. Anyway, I think we can agree that such near trivial patches shouldn't have to wait for a week?
Paul B Mahol (2018-03-20): > So user can pick which channels to extract. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > doc/filters.texi | 15 ++++++++++++ > libavfilter/af_channelsplit.c | 54 +++++++++++++++++++++++++++++++++++-------- > 2 files changed, 60 insertions(+), 9 deletions(-) Quick and incomplete review, for the sake of courtesy. > > diff --git a/doc/filters.texi b/doc/filters.texi > index bd43a7ac6e..81310e1cdf 100644 > --- a/doc/filters.texi > +++ b/doc/filters.texi > @@ -2208,8 +2208,14 @@ It accepts the following parameters: > @table @option > @item channel_layout > The channel layout of the input stream. The default is "stereo". > +@item channels > +The channel layout of the channels for extraction. The default is "all". You do not specify what happens if it specifies channels that are not present in the input. > @end table > > +@subsection Examples > + > +@itemize > +@item > For example, assuming a stereo input MP3 file, > @example > ffmpeg -i in.mp3 -filter_complex channelsplit out.mkv > @@ -2217,6 +2223,7 @@ ffmpeg -i in.mp3 -filter_complex channelsplit out.mkv > will create an output Matroska file with two audio streams, one containing only > the left channel and the other the right channel. > > +@item > Split a 5.1 WAV file into per-channel files: > @example > ffmpeg -i in.wav -filter_complex > @@ -2226,6 +2233,14 @@ front_center.wav -map '[LFE]' lfe.wav -map '[SL]' side_left.wav -map '[SR]' > side_right.wav > @end example > > +@item > +Extract LFE from a 5.1 WAV file: > +@example > +ffmpeg -i in.wav -filter_complex 'channelsplit=channel_layout=5.1:LFE[LFE]' "channel_layout=5.1:channels=LFE"; avoid giving examples relying on the order of options, especially for secondary ones. > +-map '[LFE]' lfe.wav > +@end example > +@end itemize > + > @section chorus > Add a chorus effect to the audio. > > diff --git a/libavfilter/af_channelsplit.c b/libavfilter/af_channelsplit.c > index 8c6b00fe4f..d9b9a60420 100644 > --- a/libavfilter/af_channelsplit.c > +++ b/libavfilter/af_channelsplit.c > @@ -38,6 +38,9 @@ typedef struct ChannelSplitContext { > > uint64_t channel_layout; > char *channel_layout_str; > + char *channels_str; > + > + int map[64]; Minor: could be char to save some memory. > } ChannelSplitContext; > > #define OFFSET(x) offsetof(ChannelSplitContext, x) > @@ -45,6 +48,7 @@ typedef struct ChannelSplitContext { > #define F AV_OPT_FLAG_FILTERING_PARAM > static const AVOption channelsplit_options[] = { > { "channel_layout", "Input channel layout.", OFFSET(channel_layout_str), AV_OPT_TYPE_STRING, { .str = "stereo" }, .flags = A|F }, > + { "channels", "Channels to extract.", OFFSET(channels_str), AV_OPT_TYPE_STRING, { .str = "all" }, .flags = A|F }, > { NULL } > }; > > @@ -64,15 +68,46 @@ static av_cold int init(AVFilterContext *ctx) > } > > nb_channels = av_get_channel_layout_nb_channels(s->channel_layout); > - for (i = 0; i < nb_channels; i++) { > - uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); > - AVFilterPad pad = { 0 }; > > - pad.type = AVMEDIA_TYPE_AUDIO; > - pad.name = av_get_channel_name(channel); > + if (!strcmp(s->channels_str, "all")) { > + for (i = 0; i < nb_channels; i++) { > + uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); > + AVFilterPad pad = { 0 }; > + > + pad.type = AVMEDIA_TYPE_AUDIO; > + pad.name = av_get_channel_name(channel); > + > + s->map[i] = i; > > - if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { > + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { > + return ret; > + } > + } > + } else { > + uint64_t channel_layout; > + int nb_extracted_channels; Inconsistent variable names. > + > + if ((ret = av_get_extended_channel_layout(s->channels_str, &channel_layout, &nb_extracted_channels)) < 0) > return ret; > + > + for (i = 0; i < nb_extracted_channels; i++) { > + uint64_t channel = av_channel_layout_extract_channel(channel_layout, i); > + AVFilterPad pad = { 0 }; > + > + if ((ret = av_get_channel_layout_channel_index(s->channel_layout, channel)) < 0) { > + av_log(ctx, AV_LOG_ERROR, "Channel name '%s' not present in channel layout '%s'.\n", > + av_get_channel_name(channel), s->channel_layout_str); > + return ret; > + } > + > + s->map[i] = ret; > + > + pad.type = AVMEDIA_TYPE_AUDIO; > + pad.name = av_get_channel_name(channel); > + > + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { > + return ret; > + } Stop using copy-paste! Set extracted_channels to either the parsed value or the same as s->channel_layout and build the map only once. > } > } > > @@ -96,7 +131,7 @@ static int query_formats(AVFilterContext *ctx) > > for (i = 0; i < ctx->nb_outputs; i++) { > AVFilterChannelLayouts *out_layouts = NULL; > - uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); > + uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, s->map[i]); > > if ((ret = ff_add_channel_layout(&out_layouts, channel)) < 0 || > (ret = ff_channel_layouts_ref(out_layouts, &ctx->outputs[i]->in_channel_layouts)) < 0) > @@ -109,6 +144,7 @@ static int query_formats(AVFilterContext *ctx) > static int filter_frame(AVFilterLink *inlink, AVFrame *buf) > { > AVFilterContext *ctx = inlink->dst; > + ChannelSplitContext *s = ctx->priv; > int i, ret = 0; > > for (i = 0; i < ctx->nb_outputs; i++) { > @@ -119,9 +155,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *buf) > break; > } > > - buf_out->data[0] = buf_out->extended_data[0] = buf_out->extended_data[i]; > + buf_out->data[0] = buf_out->extended_data[0] = buf_out->extended_data[s->map[i]]; > buf_out->channel_layout = > - av_channel_layout_extract_channel(buf->channel_layout, i); > + av_channel_layout_extract_channel(buf->channel_layout, s->map[i]); > buf_out->channels = 1; > > ret = ff_filter_frame(ctx->outputs[i], buf_out); Regards,
On 3/21/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-03-20): >> So user can pick which channels to extract. >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> doc/filters.texi | 15 ++++++++++++ >> libavfilter/af_channelsplit.c | 54 >> +++++++++++++++++++++++++++++++++++-------- >> 2 files changed, 60 insertions(+), 9 deletions(-) > > Quick and incomplete review, for the sake of courtesy. > >> >> diff --git a/doc/filters.texi b/doc/filters.texi >> index bd43a7ac6e..81310e1cdf 100644 >> --- a/doc/filters.texi >> +++ b/doc/filters.texi >> @@ -2208,8 +2208,14 @@ It accepts the following parameters: >> @table @option >> @item channel_layout >> The channel layout of the input stream. The default is "stereo". > >> +@item channels >> +The channel layout of the channels for extraction. The default is "all". > > You do not specify what happens if it specifies channels that are not > present in the input. OK, what happens is that it fails, and reports reason to the user. > >> @end table >> >> +@subsection Examples >> + >> +@itemize >> +@item >> For example, assuming a stereo input MP3 file, >> @example >> ffmpeg -i in.mp3 -filter_complex channelsplit out.mkv >> @@ -2217,6 +2223,7 @@ ffmpeg -i in.mp3 -filter_complex channelsplit >> out.mkv >> will create an output Matroska file with two audio streams, one >> containing only >> the left channel and the other the right channel. >> >> +@item >> Split a 5.1 WAV file into per-channel files: >> @example >> ffmpeg -i in.wav -filter_complex >> @@ -2226,6 +2233,14 @@ front_center.wav -map '[LFE]' lfe.wav -map '[SL]' >> side_left.wav -map '[SR]' >> side_right.wav >> @end example >> >> +@item >> +Extract LFE from a 5.1 WAV file: >> +@example > >> +ffmpeg -i in.wav -filter_complex >> 'channelsplit=channel_layout=5.1:LFE[LFE]' > > "channel_layout=5.1:channels=LFE"; avoid giving examples relying on > the order of options, especially for secondary ones. OK > >> +-map '[LFE]' lfe.wav >> +@end example >> +@end itemize >> + >> @section chorus >> Add a chorus effect to the audio. >> >> diff --git a/libavfilter/af_channelsplit.c >> b/libavfilter/af_channelsplit.c >> index 8c6b00fe4f..d9b9a60420 100644 >> --- a/libavfilter/af_channelsplit.c >> +++ b/libavfilter/af_channelsplit.c >> @@ -38,6 +38,9 @@ typedef struct ChannelSplitContext { >> >> uint64_t channel_layout; >> char *channel_layout_str; >> + char *channels_str; >> + > >> + int map[64]; > > Minor: could be char to save some memory. > >> } ChannelSplitContext; >> >> #define OFFSET(x) offsetof(ChannelSplitContext, x) >> @@ -45,6 +48,7 @@ typedef struct ChannelSplitContext { >> #define F AV_OPT_FLAG_FILTERING_PARAM >> static const AVOption channelsplit_options[] = { >> { "channel_layout", "Input channel layout.", >> OFFSET(channel_layout_str), AV_OPT_TYPE_STRING, { .str = "stereo" }, >> .flags = A|F }, >> + { "channels", "Channels to extract.", OFFSET(channels_str), >> AV_OPT_TYPE_STRING, { .str = "all" }, .flags = A|F }, >> { NULL } >> }; >> >> @@ -64,15 +68,46 @@ static av_cold int init(AVFilterContext *ctx) >> } >> >> nb_channels = av_get_channel_layout_nb_channels(s->channel_layout); >> - for (i = 0; i < nb_channels; i++) { >> - uint64_t channel = >> av_channel_layout_extract_channel(s->channel_layout, i); >> - AVFilterPad pad = { 0 }; >> >> - pad.type = AVMEDIA_TYPE_AUDIO; >> - pad.name = av_get_channel_name(channel); >> + if (!strcmp(s->channels_str, "all")) { >> + for (i = 0; i < nb_channels; i++) { >> + uint64_t channel = >> av_channel_layout_extract_channel(s->channel_layout, i); >> + AVFilterPad pad = { 0 }; >> + >> + pad.type = AVMEDIA_TYPE_AUDIO; >> + pad.name = av_get_channel_name(channel); >> + >> + s->map[i] = i; >> >> - if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { >> + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { >> + return ret; >> + } >> + } >> + } else { > > >> + uint64_t channel_layout; >> + int nb_extracted_channels; > > Inconsistent variable names. I dont follow. > >> + >> + if ((ret = av_get_extended_channel_layout(s->channels_str, >> &channel_layout, &nb_extracted_channels)) < 0) >> return ret; >> + >> + for (i = 0; i < nb_extracted_channels; i++) { >> + uint64_t channel = >> av_channel_layout_extract_channel(channel_layout, i); >> + AVFilterPad pad = { 0 }; >> + >> + if ((ret = >> av_get_channel_layout_channel_index(s->channel_layout, channel)) < 0) { >> + av_log(ctx, AV_LOG_ERROR, "Channel name '%s' not present >> in channel layout '%s'.\n", >> + av_get_channel_name(channel), >> s->channel_layout_str); >> + return ret; >> + } >> + >> + s->map[i] = ret; >> + >> + pad.type = AVMEDIA_TYPE_AUDIO; >> + pad.name = av_get_channel_name(channel); >> + >> + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { >> + return ret; >> + } > > Stop using copy-paste! Set extracted_channels to either the parsed value > or the same as s->channel_layout and build the map only once. Where do you see I copy pasted anything? > >> } >> } >> >> @@ -96,7 +131,7 @@ static int query_formats(AVFilterContext *ctx) >> >> for (i = 0; i < ctx->nb_outputs; i++) { >> AVFilterChannelLayouts *out_layouts = NULL; >> - uint64_t channel = >> av_channel_layout_extract_channel(s->channel_layout, i); >> + uint64_t channel = >> av_channel_layout_extract_channel(s->channel_layout, s->map[i]); >> >> if ((ret = ff_add_channel_layout(&out_layouts, channel)) < 0 || >> (ret = ff_channel_layouts_ref(out_layouts, >> &ctx->outputs[i]->in_channel_layouts)) < 0) >> @@ -109,6 +144,7 @@ static int query_formats(AVFilterContext *ctx) >> static int filter_frame(AVFilterLink *inlink, AVFrame *buf) >> { >> AVFilterContext *ctx = inlink->dst; >> + ChannelSplitContext *s = ctx->priv; >> int i, ret = 0; >> >> for (i = 0; i < ctx->nb_outputs; i++) { >> @@ -119,9 +155,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame >> *buf) >> break; >> } >> >> - buf_out->data[0] = buf_out->extended_data[0] = >> buf_out->extended_data[i]; >> + buf_out->data[0] = buf_out->extended_data[0] = >> buf_out->extended_data[s->map[i]]; >> buf_out->channel_layout = >> - av_channel_layout_extract_channel(buf->channel_layout, i); >> + av_channel_layout_extract_channel(buf->channel_layout, >> s->map[i]); >> buf_out->channels = 1; >> >> ret = ff_filter_frame(ctx->outputs[i], buf_out); > > Regards, > > -- > Nicolas George Accepted stuff fixed locally and gonna apply.
Paul B Mahol (2018-03-21): > >> + uint64_t channel_layout; > >> + int nb_extracted_channels; > > Inconsistent variable names. > I dont follow. The two variables describe the same entity, they should have the same base name: nb_extracted_channels and extracted_channels, obviously. > Where do you see I copy pasted anything? The block I quoted is almost identical to the block just before. Duplicated code is unacceptable. Factor it. > Accepted stuff fixed locally and gonna apply. Send the updated patch for review. But fix the duplicated code before. Regards,
On 3/21/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-03-21): >> >> + uint64_t channel_layout; >> >> + int nb_extracted_channels; >> > Inconsistent variable names. >> I dont follow. > > The two variables describe the same entity, they should have the same > base name: nb_extracted_channels and extracted_channels, obviously. > >> Where do you see I copy pasted anything? > > The block I quoted is almost identical to the block just before. > Duplicated code is unacceptable. Factor it. It is hardly identical, and I can not read your mind. And it is functionally incompatible. Enough bikesheds from you! > >> Accepted stuff fixed locally and gonna apply. > > Send the updated patch for review. But fix the duplicated code before. > > Regards, > > -- > Nicolas George >
Paul B Mahol (2018-03-21): > It is hardly identical, and I can not read your mind. More than half the block is identical to the previous one. > And it is functionally incompatible. I told you how you can factor it. > Enough bikesheds from you! This is a legitimate technical concern: duplicated code is bad, duplicated that is that easy to factor is unacceptable. Enhance your code or drop the patch, but do not apply as is. Regards,
On 3/21/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-03-21): >> It is hardly identical, and I can not read your mind. > > More than half the block is identical to the previous one. > >> And it is functionally incompatible. > > I told you how you can factor it. > >> Enough bikesheds from you! > > This is a legitimate technical concern: duplicated code is bad, > duplicated that is that easy to factor is unacceptable. Enhance your > code or drop the patch, but do not apply as is. I have legitimate technical concert about your faulty review. There is log message which is incompatible with whatever you propose. So from now on I'm pushing patches like this without review, expecially one frome you.
Paul B Mahol (2018-03-21): > I have legitimate technical concert about your faulty review. Seriously, have you no pride at all for the quality of your code? Personally, I would feel ashamed to publish that code, let alone defend it if somebody points the flaw to me. > There is log message which is incompatible with whatever you propose. The log message only happens if a channel is present in extracted_channels but not channel_layout. It cannot happen if extracted_channels = channel_layout. Seriously, the ONLY thing you need to do is set extracted_channels to the same value as channel_layout instead of parsing it if it is "all". Absolutely nothing more. > So from now on I'm pushing patches like this without review, > expecially one frome you. Then I'll revert and request that your commit rights be revoked. Regards,
Hi Paul! On 2018-03-20 19:11 +0100, Paul B Mahol wrote: > So user can pick which channels to extract. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > doc/filters.texi | 15 ++++++++++++ > libavfilter/af_channelsplit.c | 54 +++++++++++++++++++++++++++++++++++-------- > 2 files changed, 60 insertions(+), 9 deletions(-) > > diff --git a/doc/filters.texi b/doc/filters.texi > index bd43a7ac6e..81310e1cdf 100644 > --- a/doc/filters.texi > +++ b/doc/filters.texi > @@ -2208,8 +2208,14 @@ It accepts the following parameters: > @table @option > @item channel_layout > The channel layout of the input stream. The default is "stereo". > +@item channels > +The channel layout of the channels for extraction. The default is "all". This sentence feels a bit too in-accurate to me. Maybe like this: A channel layout describing the channels to be extracted as separate output streams or "all" to extract each input channel as a separate stream. The default is "all". While writing this, I think maybe another way to implement the default would be to set the channels to the channel_layout of the input stream. So the documentation would read: A channel layout describing the channels to be extracted as separate output streams. Defaults to the channel layout of the input stream. Would also simplify the implementation. But I may sure be missing something... > @end table > > +@subsection Examples > + > +@itemize > +@item > For example, assuming a stereo input MP3 file, > @example > ffmpeg -i in.mp3 -filter_complex channelsplit out.mkv > @@ -2217,6 +2223,7 @@ ffmpeg -i in.mp3 -filter_complex channelsplit out.mkv > will create an output Matroska file with two audio streams, one containing only > the left channel and the other the right channel. > > +@item > Split a 5.1 WAV file into per-channel files: > @example > ffmpeg -i in.wav -filter_complex > @@ -2226,6 +2233,14 @@ front_center.wav -map '[LFE]' lfe.wav -map '[SL]' side_left.wav -map '[SR]' > side_right.wav > @end example > > +@item > +Extract LFE from a 5.1 WAV file: > +@example > +ffmpeg -i in.wav -filter_complex 'channelsplit=channel_layout=5.1:LFE[LFE]' > +-map '[LFE]' lfe.wav > +@end example > +@end itemize > + > @section chorus > Add a chorus effect to the audio. > > diff --git a/libavfilter/af_channelsplit.c b/libavfilter/af_channelsplit.c > index 8c6b00fe4f..d9b9a60420 100644 > --- a/libavfilter/af_channelsplit.c > +++ b/libavfilter/af_channelsplit.c > @@ -38,6 +38,9 @@ typedef struct ChannelSplitContext { > > uint64_t channel_layout; > char *channel_layout_str; > + char *channels_str; > + > + int map[64]; > } ChannelSplitContext; > > #define OFFSET(x) offsetof(ChannelSplitContext, x) > @@ -45,6 +48,7 @@ typedef struct ChannelSplitContext { > #define F AV_OPT_FLAG_FILTERING_PARAM > static const AVOption channelsplit_options[] = { > { "channel_layout", "Input channel layout.", OFFSET(channel_layout_str), AV_OPT_TYPE_STRING, { .str = "stereo" }, .flags = A|F }, > + { "channels", "Channels to extract.", OFFSET(channels_str), AV_OPT_TYPE_STRING, { .str = "all" }, .flags = A|F }, > { NULL } > }; > > @@ -64,15 +68,46 @@ static av_cold int init(AVFilterContext *ctx) > } > > nb_channels = av_get_channel_layout_nb_channels(s->channel_layout); > - for (i = 0; i < nb_channels; i++) { > - uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); > - AVFilterPad pad = { 0 }; > > - pad.type = AVMEDIA_TYPE_AUDIO; > - pad.name = av_get_channel_name(channel); > + if (!strcmp(s->channels_str, "all")) { > + for (i = 0; i < nb_channels; i++) { > + uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); > + AVFilterPad pad = { 0 }; > + > + pad.type = AVMEDIA_TYPE_AUDIO; > + pad.name = av_get_channel_name(channel); > + > + s->map[i] = i; > > - if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { > + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { > + return ret; > + } > + } > + } else { > + uint64_t channel_layout; > + int nb_extracted_channels; Checking nb_channels above and nb_extracted_channels here against length of s->map would be more robust. > + > + if ((ret = av_get_extended_channel_layout(s->channels_str, &channel_layout, &nb_extracted_channels)) < 0) > return ret; > + > + for (i = 0; i < nb_extracted_channels; i++) { > + uint64_t channel = av_channel_layout_extract_channel(channel_layout, i); > + AVFilterPad pad = { 0 }; > + > + if ((ret = av_get_channel_layout_channel_index(s->channel_layout, channel)) < 0) { > + av_log(ctx, AV_LOG_ERROR, "Channel name '%s' not present in channel layout '%s'.\n", > + av_get_channel_name(channel), s->channel_layout_str); > + return ret; > + } > + > + s->map[i] = ret; > + > + pad.type = AVMEDIA_TYPE_AUDIO; > + pad.name = av_get_channel_name(channel); Nit: If these assignments are moved above, you could use pad.name in the av_log before the error return. I do not really care, but in case you prefer it too... > + > + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { > + return ret; > + } > } > } > > @@ -96,7 +131,7 @@ static int query_formats(AVFilterContext *ctx) > > for (i = 0; i < ctx->nb_outputs; i++) { > AVFilterChannelLayouts *out_layouts = NULL; > - uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); > + uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, s->map[i]); > > if ((ret = ff_add_channel_layout(&out_layouts, channel)) < 0 || > (ret = ff_channel_layouts_ref(out_layouts, &ctx->outputs[i]->in_channel_layouts)) < 0) > @@ -109,6 +144,7 @@ static int query_formats(AVFilterContext *ctx) > static int filter_frame(AVFilterLink *inlink, AVFrame *buf) > { > AVFilterContext *ctx = inlink->dst; > + ChannelSplitContext *s = ctx->priv; > int i, ret = 0; > > for (i = 0; i < ctx->nb_outputs; i++) { > @@ -119,9 +155,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *buf) > break; > } > > - buf_out->data[0] = buf_out->extended_data[0] = buf_out->extended_data[i]; > + buf_out->data[0] = buf_out->extended_data[0] = buf_out->extended_data[s->map[i]]; > buf_out->channel_layout = > - av_channel_layout_extract_channel(buf->channel_layout, i); > + av_channel_layout_extract_channel(buf->channel_layout, s->map[i]); > buf_out->channels = 1; > > ret = ff_filter_frame(ctx->outputs[i], buf_out); > -- Looks like a good improvement for the channelsplit filter. I only read the patch; I didn't test. Alexander
diff --git a/doc/filters.texi b/doc/filters.texi index bd43a7ac6e..81310e1cdf 100644 --- a/doc/filters.texi +++ b/doc/filters.texi @@ -2208,8 +2208,14 @@ It accepts the following parameters: @table @option @item channel_layout The channel layout of the input stream. The default is "stereo". +@item channels +The channel layout of the channels for extraction. The default is "all". @end table +@subsection Examples + +@itemize +@item For example, assuming a stereo input MP3 file, @example ffmpeg -i in.mp3 -filter_complex channelsplit out.mkv @@ -2217,6 +2223,7 @@ ffmpeg -i in.mp3 -filter_complex channelsplit out.mkv will create an output Matroska file with two audio streams, one containing only the left channel and the other the right channel. +@item Split a 5.1 WAV file into per-channel files: @example ffmpeg -i in.wav -filter_complex @@ -2226,6 +2233,14 @@ front_center.wav -map '[LFE]' lfe.wav -map '[SL]' side_left.wav -map '[SR]' side_right.wav @end example +@item +Extract LFE from a 5.1 WAV file: +@example +ffmpeg -i in.wav -filter_complex 'channelsplit=channel_layout=5.1:LFE[LFE]' +-map '[LFE]' lfe.wav +@end example +@end itemize + @section chorus Add a chorus effect to the audio. diff --git a/libavfilter/af_channelsplit.c b/libavfilter/af_channelsplit.c index 8c6b00fe4f..d9b9a60420 100644 --- a/libavfilter/af_channelsplit.c +++ b/libavfilter/af_channelsplit.c @@ -38,6 +38,9 @@ typedef struct ChannelSplitContext { uint64_t channel_layout; char *channel_layout_str; + char *channels_str; + + int map[64]; } ChannelSplitContext; #define OFFSET(x) offsetof(ChannelSplitContext, x) @@ -45,6 +48,7 @@ typedef struct ChannelSplitContext { #define F AV_OPT_FLAG_FILTERING_PARAM static const AVOption channelsplit_options[] = { { "channel_layout", "Input channel layout.", OFFSET(channel_layout_str), AV_OPT_TYPE_STRING, { .str = "stereo" }, .flags = A|F }, + { "channels", "Channels to extract.", OFFSET(channels_str), AV_OPT_TYPE_STRING, { .str = "all" }, .flags = A|F }, { NULL } }; @@ -64,15 +68,46 @@ static av_cold int init(AVFilterContext *ctx) } nb_channels = av_get_channel_layout_nb_channels(s->channel_layout); - for (i = 0; i < nb_channels; i++) { - uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); - AVFilterPad pad = { 0 }; - pad.type = AVMEDIA_TYPE_AUDIO; - pad.name = av_get_channel_name(channel); + if (!strcmp(s->channels_str, "all")) { + for (i = 0; i < nb_channels; i++) { + uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); + AVFilterPad pad = { 0 }; + + pad.type = AVMEDIA_TYPE_AUDIO; + pad.name = av_get_channel_name(channel); + + s->map[i] = i; - if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { + return ret; + } + } + } else { + uint64_t channel_layout; + int nb_extracted_channels; + + if ((ret = av_get_extended_channel_layout(s->channels_str, &channel_layout, &nb_extracted_channels)) < 0) return ret; + + for (i = 0; i < nb_extracted_channels; i++) { + uint64_t channel = av_channel_layout_extract_channel(channel_layout, i); + AVFilterPad pad = { 0 }; + + if ((ret = av_get_channel_layout_channel_index(s->channel_layout, channel)) < 0) { + av_log(ctx, AV_LOG_ERROR, "Channel name '%s' not present in channel layout '%s'.\n", + av_get_channel_name(channel), s->channel_layout_str); + return ret; + } + + s->map[i] = ret; + + pad.type = AVMEDIA_TYPE_AUDIO; + pad.name = av_get_channel_name(channel); + + if ((ret = ff_insert_outpad(ctx, i, &pad)) < 0) { + return ret; + } } } @@ -96,7 +131,7 @@ static int query_formats(AVFilterContext *ctx) for (i = 0; i < ctx->nb_outputs; i++) { AVFilterChannelLayouts *out_layouts = NULL; - uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, i); + uint64_t channel = av_channel_layout_extract_channel(s->channel_layout, s->map[i]); if ((ret = ff_add_channel_layout(&out_layouts, channel)) < 0 || (ret = ff_channel_layouts_ref(out_layouts, &ctx->outputs[i]->in_channel_layouts)) < 0) @@ -109,6 +144,7 @@ static int query_formats(AVFilterContext *ctx) static int filter_frame(AVFilterLink *inlink, AVFrame *buf) { AVFilterContext *ctx = inlink->dst; + ChannelSplitContext *s = ctx->priv; int i, ret = 0; for (i = 0; i < ctx->nb_outputs; i++) { @@ -119,9 +155,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *buf) break; } - buf_out->data[0] = buf_out->extended_data[0] = buf_out->extended_data[i]; + buf_out->data[0] = buf_out->extended_data[0] = buf_out->extended_data[s->map[i]]; buf_out->channel_layout = - av_channel_layout_extract_channel(buf->channel_layout, i); + av_channel_layout_extract_channel(buf->channel_layout, s->map[i]); buf_out->channels = 1; ret = ff_filter_frame(ctx->outputs[i], buf_out);
So user can pick which channels to extract. Signed-off-by: Paul B Mahol <onemda@gmail.com> --- doc/filters.texi | 15 ++++++++++++ libavfilter/af_channelsplit.c | 54 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 9 deletions(-)