On 3/21/18, Nicolas George <geo...@nsup.org> wrote:
> Paul B Mahol (2018-03-20):
>> So user can pick which channels to extract.
>>
>> Signed-off-by: Paul B Mahol <one...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
