[FFmpeg-devel,4/4] ffplay: do not set redundant channel count on abuffersink.

Signed-off-by: Nicolas George <george@nsup.org>
---
 fftools/ffplay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 8/14/20, Nicolas George <george@nsup.org> wrote:
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  fftools/ffplay.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Looks OK, but I'm not maintainer of this code.

On Fri, 14 Aug 2020, Nicolas George wrote:

> Signed-off-by: Nicolas George <george@nsup.org>
> ---
> fftools/ffplay.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index d673b8049a..6c9c041e9a 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -2008,7 +2008,7 @@ static int configure_audio_filters(VideoState *is, const char *afilters, int for
>
>     if (force_output_format) {
>         channel_layouts[0] = is->audio_tgt.channel_layout;
> -        channels       [0] = is->audio_tgt.channels;
> +        channels       [0] = is->audio_tgt.channel_layout ? -1 : is->audio_tgt.channels;

Why it is better if we don't set it?

Thanks,
Marton

Marton Balint (12020-08-20):
> Why it is better if we don't set it?

It is how the API was supposed to be used from the start: these values
set the formats list for buffersink. If you said you accept anything
with two channels, then you do not need to say you accept stereo, it is
already said. It was already documented in formats.h:

 * - The list must not contain a layout with a known disposition and a
 *   channel count with unknown disposition with the same number of channels
 *   (e.g. AV_CH_LAYOUT_STEREO and FF_COUNT2LAYOUT(2).

Of course, a private header is not the proper place to document a public
API, which is what I correct in patch #2.

In practice, applications know if they want a number of channels or a
specific channel layout, and should only set one. In this case I am not
familiar enough with SDL's API to be sure, so I made the minimal change.

Regards,

On Thu, 20 Aug 2020, Nicolas George wrote:

> Marton Balint (12020-08-20):
>> Why it is better if we don't set it?
>
> It is how the API was supposed to be used from the start: these values
> set the formats list for buffersink. If you said you accept anything
> with two channels, then you do not need to say you accept stereo, it is
> already said. It was already documented in formats.h:
>
> * - The list must not contain a layout with a known disposition and a
> *   channel count with unknown disposition with the same number of channels
> *   (e.g. AV_CH_LAYOUT_STEREO and FF_COUNT2LAYOUT(2).
>
> Of course, a private header is not the proper place to document a public
> API, which is what I correct in patch #2.
>
> In practice, applications know if they want a number of channels or a
> specific channel layout, and should only set one. In this case I am not
> familiar enough with SDL's API to be sure, so I made the minimal change.

LGTM, thanks.

Regards,
Marton

Marton Balint (12020-08-20):
> > It is how the API was supposed to be used from the start: these values
> > set the formats list for buffersink. If you said you accept anything
> > with two channels, then you do not need to say you accept stereo, it is
> > already said. It was already documented in formats.h:
> > 
> > * - The list must not contain a layout with a known disposition and a
> > *   channel count with unknown disposition with the same number of channels
> > *   (e.g. AV_CH_LAYOUT_STEREO and FF_COUNT2LAYOUT(2).
> > 
> > Of course, a private header is not the proper place to document a public
> > API, which is what I correct in patch #2.
> > 
> > In practice, applications know if they want a number of channels or a
> > specific channel layout, and should only set one. In this case I am not
> > familiar enough with SDL's API to be sure, so I made the minimal change.
> 
> LGTM, thanks.

Thanks, pushed.

Regards,