diff mbox series

[FFmpeg-devel,1/5] avutil/channel_layout: add 22.2 layout

Message ID 20200616210253.7169-2-jeebjp@gmail.com
State Superseded
Headers show
Series 22.2 channel layout support for AAC decoding
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström June 16, 2020, 9:02 p.m. UTC
Requires some extraneous top side and bottom front channels to be
defined.

According to STD-B59v2, the defined channel layout is:
- FL
- FR
- FC
- LFE1
- BL
- BR
- FLc
- FRc
- BC
- LFE2
- SiL
- SiR
- TpFL
- TpFR
- TpFC
- TpC
- TpBL
- TpBR
- TpSiL
- TpSiR
- TpBC
- BtFC
- BtFL
- BtFR
---
 doc/APIchanges             | 5 +++++
 libavutil/channel_layout.c | 6 ++++++
 libavutil/channel_layout.h | 6 ++++++
 libavutil/version.h        | 2 +-
 4 files changed, 18 insertions(+), 1 deletion(-)

Comments

Paul B Mahol Aug. 1, 2020, 11:46 a.m. UTC | #1
On 6/16/20, Jan Ekström <jeebjp@gmail.com> wrote:
> Requires some extraneous top side and bottom front channels to be
> defined.
>
> According to STD-B59v2, the defined channel layout is:
> - FL
> - FR
> - FC
> - LFE1
> - BL
> - BR
> - FLc
> - FRc
> - BC
> - LFE2
> - SiL
> - SiR
> - TpFL
> - TpFR
> - TpFC
> - TpC
> - TpBL
> - TpBR
> - TpSiL
> - TpSiR
> - TpBC
> - BtFC
> - BtFL
> - BtFR
> ---
>  doc/APIchanges             | 5 +++++
>  libavutil/channel_layout.c | 6 ++++++
>  libavutil/channel_layout.h | 6 ++++++
>  libavutil/version.h        | 2 +-
>  4 files changed, 18 insertions(+), 1 deletion(-)
>

probably OK

BTW What happened with new channel layout API?
Carl Eugen Hoyos Aug. 1, 2020, 11:51 a.m. UTC | #2
Am Di., 16. Juni 2020 um 23:11 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>
> Requires some extraneous top side and bottom front channels to be
> defined.
>
> According to STD-B59v2, the defined channel layout is:
> - FL
> - FR
> - FC
> - LFE1
> - BL
> - BR

> - FLc
> - FRc
> - BC
> - LFE2
> - SiL
> - SiR

No way to change the order to make it compatible with 7.1?

Carl Eugen
Jan Ekström Aug. 1, 2020, 12:06 p.m. UTC | #3
On Sat, Aug 1, 2020 at 2:51 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Di., 16. Juni 2020 um 23:11 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >
> > Requires some extraneous top side and bottom front channels to be
> > defined.
> >
> > According to STD-B59v2, the defined channel layout is:
> > - FL
> > - FR
> > - FC
> > - LFE1
> > - BL
> > - BR
>
> > - FLc
> > - FRc
> > - BC
> > - LFE2
> > - SiL
> > - SiR
>
> No way to change the order to make it compatible with 7.1?
>

I don't think that's realistic since then if we would like our actual
22.2 output to be compatible with the standard-defined channel order
we would have to have a forced conversion at an end user facing end
point, while having another one for the programmatic end points.

Additionally, the spec only mentions stereo/5.1 for direct mapping, so
I've kept to 5.1 since we have the capability for that one.

The specification is available in English @
http://www.arib.or.jp/english/html/overview/doc/6-STD-B59v2_0-E1.pdf .

Best regards,
Jan
Carl Eugen Hoyos Aug. 1, 2020, 12:08 p.m. UTC | #4
Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:

> Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> I've kept to 5.1 since we have the capability for that one.

And you still believe it would be a disadvantage if the decoder outputs
this 5.1 mapping by default if nothing else was requested?

Is there an option to request the 5.1 mapping?

Carl Eugen
Jan Ekström Aug. 1, 2020, 12:17 p.m. UTC | #5
On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>
> > Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> > I've kept to 5.1 since we have the capability for that one.
>
> And you still believe it would be a disadvantage if the decoder outputs
> this 5.1 mapping by default if nothing else was requested?
>

Yes. API users so far have received the audio frames according to
encoded layout by default, so doing something else breaks that rule of
least surprises.

> Is there an option to request the 5.1 mapping?
>

As noted in the 0/0 cover letter, you can do it just fine with
`-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
the 5.1 channels as-is.

At this point I would not like to attempt to figure out how to skip
channel coding units in the AAC decoder, as swresample seems to handle
this nicely. So far the only usage for requested channel layout in
aacdec has been to skip reordering with AV_CH_LAYOUT_NATIVE , to
output the coded units as-is.

Best regards,
Jan
Carl Eugen Hoyos Aug. 1, 2020, 1:04 p.m. UTC | #6
Am Sa., 1. Aug. 2020 um 14:47 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>
> On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >
> > Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >
> > > Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> > > I've kept to 5.1 since we have the capability for that one.
> >
> > And you still believe it would be a disadvantage if the decoder outputs
> > this 5.1 mapping by default if nothing else was requested?
>
> Yes. API users so far have received the audio frames according to
> encoded layout by default, so doing something else breaks that rule of
> least surprises.
>
> > Is there an option to request the 5.1 mapping?
>
> As noted in the 0/0 cover letter, you can do it just fine with
> `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
> the 5.1 channels as-is.

But this does not make the decoder output 5.1 or does it?

While I should probably only care about ffmpeg and ignore
the library users I still wonder who can live with your approach...

> At this point I would not like to attempt to figure out how to skip
> channel coding units in the AAC decoder

Just throw them away?

Thank you for your effort, Carl Eugen
James Almer Aug. 1, 2020, 1:42 p.m. UTC | #7
On 8/1/2020 9:17 AM, Jan Ekström wrote:
> On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>> Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>>
>>> Additionally, the spec only mentions stereo/5.1 for direct mapping, so
>>> I've kept to 5.1 since we have the capability for that one.
>>
>> And you still believe it would be a disadvantage if the decoder outputs
>> this 5.1 mapping by default if nothing else was requested?
>>
> 
> Yes. API users so far have received the audio frames according to
> encoded layout by default, so doing something else breaks that rule of
> least surprises.
> 
>> Is there an option to request the 5.1 mapping?
>>
> 
> As noted in the 0/0 cover letter, you can do it just fine with
> `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
> the 5.1 channels as-is.

I assume that does a "conversion" using swr from 22.2 to 5.1? (even if
it just outputs the first six channels without extra resampling). What
Carl wants is the decoder outputting only the first six channels,
without the need for external resampling. This should be achievable with
the -request_channel_layout input option (Try it with the DTS decoder on
DTS-MA 7.1 samples to get 5.1, or in some DTS core samples to get
stereo). No external conversion takes place in this scenario, since the
decoder takes the hint to only output the requested channels, assuming
it's doable (In the DTS case, the stream may not have a defined way
within the bitstream to achieve this, and so the decoder just outputs
the original channel layout silently).

This would of course be separate work, but IMO very welcome and worth a
look, and not a blocker. In any case, the decoder should by default
output what the stream reports it contains, and not some arbitrary
layout we think a CLI or player user will prefer. The user can and is
expected to use a resampling library for that.

> 
> At this point I would not like to attempt to figure out how to skip
> channel coding units in the AAC decoder, as swresample seems to handle
> this nicely. So far the only usage for requested channel layout in
> aacdec has been to skip reordering with AV_CH_LAYOUT_NATIVE , to
> output the coded units as-is.
> 
> Best regards,
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Jan Ekström Aug. 1, 2020, 1:44 p.m. UTC | #8
On Sat, Aug 1, 2020 at 4:29 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Sa., 1. Aug. 2020 um 14:47 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >
> > On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > >
> > > Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> > >
> > > > Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> > > > I've kept to 5.1 since we have the capability for that one.
> > >
> > > And you still believe it would be a disadvantage if the decoder outputs
> > > this 5.1 mapping by default if nothing else was requested?
> >
> > Yes. API users so far have received the audio frames according to
> > encoded layout by default, so doing something else breaks that rule of
> > least surprises.
> >
> > > Is there an option to request the 5.1 mapping?
> >
> > As noted in the 0/0 cover letter, you can do it just fine with
> > `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
> > the 5.1 channels as-is.
>
> But this does not make the decoder output 5.1 or does it?
>
> While I should probably only care about ffmpeg and ignore
> the library users I still wonder who can live with your approach...
>

I do actually think of the API users, or at least how I see API users
around my point of vision (aka "what I can see from where I am
currently"). You are of course free to note of other users, and how
they see things problematically.

I think from the decoding API, the API users will expect whatever is
coded in the file, just like with video 12bit YCbCr it is being
decoded to 12bit YCbCr and not auto-converted to 8bit YCbCr just so
that it is simpler to handle by most applications (even if 10bit YCbCr
is becoming popular, 12 bit is still rather rare - just like 22.2).
And such decoded AVFrames then require swscale or some libavfilter
filter to dither it down to some specific bit depth and/or to 8bit
RGB. Although to be honest by the amount of actual broadcast samples
I've received of 22.2 AAC at this point 22.2 might be more common than
non-community made 12bit YCbCr encodes. Feel free to replace 12bit
YCbCr with 10bit YCbCr if you wish.

Applications which have their own audio frame thing and have a manual
#define MAX_CHANNEL_COUNT 8 on their own side will fail their own
sanity checks, yes. I know I will quite likely be poking at one API
user which currently has such a limit after this work goes in. That
said, this same issue would have happened each time larger channel
layout support was added.

I know there used to be a remix/resampling-included API before in the
decoding side of things. That way if an application knew it always
wanted X, it could request that with a single API, and not care what
was actually there in the bit stream. I can see the usefulness of
that. Granted, now that decoding is decoding, and remix is in its own
thing, most API users already have moved to either our remixing
library, or remixing through lavfi, or to a 3rd party remixer. I think
having a discussion on a higher level API like ffms2 provides is worth
it, but then again I'm not sure if it should be straddled on this
patch set.

> > At this point I would not like to attempt to figure out how to skip
> > channel coding units in the AAC decoder
>
> Just throw them away?
>

Very easy to say in words, but I am not sure how simple this is in the
code. That said, if you are interested in implementing this in the
decoder, I am not opposed to the idea since in this specific case the
spec seems to note that 5.1 can be mapped into those channels.

> Thank you for your effort, Carl Eugen

Thank you.

Jan
Jan Ekström Aug. 1, 2020, 1:51 p.m. UTC | #9
On Sat, Aug 1, 2020 at 4:42 PM James Almer <jamrial@gmail.com> wrote:
>
> On 8/1/2020 9:17 AM, Jan Ekström wrote:
> > On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >>
> >> Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >>
> >>> Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> >>> I've kept to 5.1 since we have the capability for that one.
> >>
> >> And you still believe it would be a disadvantage if the decoder outputs
> >> this 5.1 mapping by default if nothing else was requested?
> >>
> >
> > Yes. API users so far have received the audio frames according to
> > encoded layout by default, so doing something else breaks that rule of
> > least surprises.
> >
> >> Is there an option to request the 5.1 mapping?
> >>
> >
> > As noted in the 0/0 cover letter, you can do it just fine with
> > `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
> > the 5.1 channels as-is.
>
> I assume that does a "conversion" using swr from 22.2 to 5.1? (even if
> it just outputs the first six channels without extra resampling). What
> Carl wants is the decoder outputting only the first six channels,
> without the need for external resampling. This should be achievable with
> the -request_channel_layout input option (Try it with the DTS decoder on
> DTS-MA 7.1 samples to get 5.1, or in some DTS core samples to get
> stereo). No external conversion takes place in this scenario, since the
> decoder takes the hint to only output the requested channels, assuming
> it's doable (In the DTS case, the stream may not have a defined way
> within the bitstream to achieve this, and so the decoder just outputs
> the original channel layout silently).
>
> This would of course be separate work, but IMO very welcome and worth a
> look, and not a blocker. In any case, the decoder should by default
> output what the stream reports it contains, and not some arbitrary
> layout we think a CLI or player user will prefer. The user can and is
> expected to use a resampling library for that.
>

Aye. I know of that feature, and I actually did utilize
`-request_channel_layout 9223372036854775808` for testing my decoder
work (It is the only usage of that feature in the AAC decoder, it
skips the coding unit reordering. That feature still works even with
my changes).

I do agree that it could be implemented, but I'm not sure if it should
be part of this patch set which makes:
1. The streams demux and decode'able (unblocks remux)
2. Seems to output the correct 22.2 channel layout. (so that you can
dump the 22.2 into WAV or otherwise utilize it through the API by
default)
3. Can be utilized as output for 5.1 or stereo by utilizing swresample
(if input is 22.2 and output is not 22.2 then it gets handled as 5.1 -
at least until someone decides to make a proper mixing matrix
configuration for it).

I would actually prefer comments on how the swresample changes were
done. They led to correct results for me, but this is the first time
I'm poking swresample, so I might have missed something obvious.

Best regards,
Jan
Jan Ekström Aug. 2, 2020, 2:06 p.m. UTC | #10
On Sat, Aug 1, 2020 at 2:46 PM Paul B Mahol <onemda@gmail.com> wrote:
>
> On 6/16/20, Jan Ekström <jeebjp@gmail.com> wrote:
> > Requires some extraneous top side and bottom front channels to be
> > defined.
> >
> > According to STD-B59v2, the defined channel layout is:
> > - FL
> > - FR
> > - FC
> > - LFE1
> > - BL
> > - BR
> > - FLc
> > - FRc
> > - BC
> > - LFE2
> > - SiL
> > - SiR
> > - TpFL
> > - TpFR
> > - TpFC
> > - TpC
> > - TpBL
> > - TpBR
> > - TpSiL
> > - TpSiR
> > - TpBC
> > - BtFC
> > - BtFL
> > - BtFR
> > ---
> >  doc/APIchanges             | 5 +++++
> >  libavutil/channel_layout.c | 6 ++++++
> >  libavutil/channel_layout.h | 6 ++++++
> >  libavutil/version.h        | 2 +-
> >  4 files changed, 18 insertions(+), 1 deletion(-)
> >
>
> probably OK
>
> BTW What happened with new channel layout API?

Thanks for taking a look.

I think I've seen it mentioned recently when I've been discussing this
patch set, so I have a feeling we might see a new version of that at
some point. We already had multiple channels which were outside of
WAVEFORMATEXTENSIBLE (such as LFE2), so a more defining API is clearly
going to be more needed in the future (since the channels aren't
always in the rising order of definition due to how things were
defined age-wise).

Jan
Jan Ekström Aug. 2, 2020, 2:22 p.m. UTC | #11
On Sat, Aug 1, 2020 at 4:44 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> > > At this point I would not like to attempt to figure out how to skip
> > > channel coding units in the AAC decoder
> >
> > Just throw them away?
> >
>
> Very easy to say in words, but I am not sure how simple this is in the
> code. That said, if you are interested in implementing this in the
> decoder, I am not opposed to the idea since in this specific case the
> spec seems to note that 5.1 can be mapped into those channels.
>

I took a quick look at output_configure in aacdec_template, and it
isn't too clear what's the best way of getting this done.

The more proper way would seem to be to see if sniff_channel_order
returned a layout, and then proceed to applying a switch/case thing
for request_channel_layout. But more looking into the decoder would be
required to figure out if one can get it done with:
A. Just zero out mappings past the needed coding units.
B. A and resetting the tags count within output_configure (since that
is a parameter received from an outer function)
C. Something else?

Alternatively, something similar to B could get received by having the
switch/case just before the get_new_frame check, and just overriding
the layout and channels variables.

The problem is that I'm not sure how well the best working alternative
of these will work:
A. Not at all, since it will attempt to pull in data for 24 channels
but the AVFrame only contains 6. Segfault.
B. Kind of. 24 channels will get allocated, but layout is 5.1.
C. ???

I hope this clears up why I'm not too interested in trying to figure
this out right now, since I have just been able to figure out the base
decoding (which is currently everything the decoder supports!) and
downmix through swresample.

Jan
Nicolas George Aug. 2, 2020, 2:37 p.m. UTC | #12
Jan Ekström (12020-08-02):
> I think I've seen it mentioned recently when I've been discussing this
> patch set, so I have a feeling we might see a new version of that at
> some point. We already had multiple channels which were outside of
> WAVEFORMATEXTENSIBLE (such as LFE2), so a more defining API is clearly
> going to be more needed in the future (since the channels aren't
> always in the rising order of definition due to how things were
> defined age-wise).

I did not see anything recently. As far as I remember, the proposal had
serious limitations that must be addressed before it can be accepted:

- It has nothing to handle cases where the same channel is present twice
  in the layout. For example, no syntax for the user to specify "FL, but
  the second one".

- It has nothing to set extra labels and information about each channel.

As it is, it is more complex than our current very simple bit mask but
does not bring real benefit over it. As such, it is not worth it.

Regards,
James Almer Aug. 2, 2020, 2:40 p.m. UTC | #13
On 8/2/2020 11:22 AM, Jan Ekström wrote:
> On Sat, Aug 1, 2020 at 4:44 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>>>> At this point I would not like to attempt to figure out how to skip
>>>> channel coding units in the AAC decoder
>>>
>>> Just throw them away?
>>>
>>
>> Very easy to say in words, but I am not sure how simple this is in the
>> code. That said, if you are interested in implementing this in the
>> decoder, I am not opposed to the idea since in this specific case the
>> spec seems to note that 5.1 can be mapped into those channels.
>>
> 
> I took a quick look at output_configure in aacdec_template, and it
> isn't too clear what's the best way of getting this done.
> 
> The more proper way would seem to be to see if sniff_channel_order
> returned a layout, and then proceed to applying a switch/case thing
> for request_channel_layout. But more looking into the decoder would be
> required to figure out if one can get it done with:
> A. Just zero out mappings past the needed coding units.
> B. A and resetting the tags count within output_configure (since that
> is a parameter received from an outer function)
> C. Something else?
> 
> Alternatively, something similar to B could get received by having the
> switch/case just before the get_new_frame check, and just overriding
> the layout and channels variables.
> 
> The problem is that I'm not sure how well the best working alternative
> of these will work:
> A. Not at all, since it will attempt to pull in data for 24 channels
> but the AVFrame only contains 6. Segfault.
> B. Kind of. 24 channels will get allocated, but layout is 5.1.
> C. ???
> 
> I hope this clears up why I'm not too interested in trying to figure
> this out right now, since I have just been able to figure out the base
> decoding (which is currently everything the decoder supports!) and
> downmix through swresample.

As i said, that's fine for now. request_channel_layout implementations
are optional and not always feasible (Very few audio codecs truly define
a way to implement it, like it's the case of DTS). The decoder's job is
to output what the stream contains, and a resampler can take care of
everything afterwards should the user want something that's not defined
within the bitstream.
Jan Ekström Aug. 2, 2020, 9:16 p.m. UTC | #14
On Sat, Aug 1, 2020 at 4:29 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Sa., 1. Aug. 2020 um 14:47 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >
> > On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > >
> > > Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> > >
> > > > Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> > > > I've kept to 5.1 since we have the capability for that one.
> > >
> > > And you still believe it would be a disadvantage if the decoder outputs
> > > this 5.1 mapping by default if nothing else was requested?
> >
> > Yes. API users so far have received the audio frames according to
> > encoded layout by default, so doing something else breaks that rule of
> > least surprises.
> >
> > > Is there an option to request the 5.1 mapping?
> >
> > As noted in the 0/0 cover letter, you can do it just fine with
> > `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
> > the 5.1 channels as-is.
>
> But this does not make the decoder output 5.1 or does it?
>
> While I should probably only care about ffmpeg and ignore
> the library users I still wonder who can live with your approach...
>

As this can be implied to be a comment regarding me not caring about
the API users, and generally being wording that can be interpreted as
disagreeing on the whole approach of the patch set, please respond
whether this is a blocker or not.

I'd rather get some closure on this one way or another, than stressing
over whether this review was a blocker or not.

Thank you,
Jan
Carl Eugen Hoyos Aug. 3, 2020, 7:03 a.m. UTC | #15
> Am 02.08.2020 um 23:16 schrieb Jan Ekström <jeebjp@gmail.com>:
> 
>> On Sat, Aug 1, 2020 at 4:29 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 
>>> Am Sa., 1. Aug. 2020 um 14:47 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>>> 
>>>> On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>> 
>>>>> Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>>>>> 
>>>>> Additionally, the spec only mentions stereo/5.1 for direct mapping, so
>>>>> I've kept to 5.1 since we have the capability for that one.
>>>> 
>>>> And you still believe it would be a disadvantage if the decoder outputs
>>>> this 5.1 mapping by default if nothing else was requested?
>>> 
>>> Yes. API users so far have received the audio frames according to
>>> encoded layout by default, so doing something else breaks that rule of
>>> least surprises.
>>> 
>>>> Is there an option to request the 5.1 mapping?
>>> 
>>> As noted in the 0/0 cover letter, you can do it just fine with
>>> `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
>>> the 5.1 channels as-is.
>> 
>> But this does not make the decoder output 5.1 or does it?
>> 
>> While I should probably only care about ffmpeg and ignore
>> the library users I still wonder who can live with your approach...
>> 
> 
> As this can be implied to be a comment regarding me not caring about
> the API users,

It was a comment about me not understanding how the average of hundreds of library users will be able to use this - important - new feature.

> and generally being wording that can be interpreted as
> disagreeing on the whole approach of the patch set, please respond
> whether this is a blocker or not.

Definitely not and I am sorry if my wording was so bad.

Carl Eugen
Hendrik Leppkes Aug. 3, 2020, 9:14 a.m. UTC | #16
On Mon, Aug 3, 2020 at 9:34 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>
>
> > Am 02.08.2020 um 23:16 schrieb Jan Ekström <jeebjp@gmail.com>:
> >
> >> On Sat, Aug 1, 2020 at 4:29 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >>
> >>> Am Sa., 1. Aug. 2020 um 14:47 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >>>
> >>>> On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >>>>
> >>>>> Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >>>>>
> >>>>> Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> >>>>> I've kept to 5.1 since we have the capability for that one.
> >>>>
> >>>> And you still believe it would be a disadvantage if the decoder outputs
> >>>> this 5.1 mapping by default if nothing else was requested?
> >>>
> >>> Yes. API users so far have received the audio frames according to
> >>> encoded layout by default, so doing something else breaks that rule of
> >>> least surprises.
> >>>
> >>>> Is there an option to request the 5.1 mapping?
> >>>
> >>> As noted in the 0/0 cover letter, you can do it just fine with
> >>> `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
> >>> the 5.1 channels as-is.
> >>
> >> But this does not make the decoder output 5.1 or does it?
> >>
> >> While I should probably only care about ffmpeg and ignore
> >> the library users I still wonder who can live with your approach...
> >>
> >
> > As this can be implied to be a comment regarding me not caring about
> > the API users,
>
> It was a comment about me not understanding how the average of hundreds of library users will be able to use this - important - new feature.
>

They can use it like they use any other uncommon channel layout, by
using lavfi or swresample directly to remix the audio - the same
process that ffmpeg.c uses internally.

- Hendrik
Jan Ekström Aug. 3, 2020, 10:13 a.m. UTC | #17
On Mon, Aug 3, 2020 at 10:34 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>
>
> > Am 02.08.2020 um 23:16 schrieb Jan Ekström <jeebjp@gmail.com>:
> >
> >> On Sat, Aug 1, 2020 at 4:29 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >>
> >>> Am Sa., 1. Aug. 2020 um 14:47 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >>>
> >>>> On Sat, Aug 1, 2020 at 3:08 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >>>>
> >>>>> Am Sa., 1. Aug. 2020 um 14:06 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >>>>>
> >>>>> Additionally, the spec only mentions stereo/5.1 for direct mapping, so
> >>>>> I've kept to 5.1 since we have the capability for that one.
> >>>>
> >>>> And you still believe it would be a disadvantage if the decoder outputs
> >>>> this 5.1 mapping by default if nothing else was requested?
> >>>
> >>> Yes. API users so far have received the audio frames according to
> >>> encoded layout by default, so doing something else breaks that rule of
> >>> least surprises.
> >>>
> >>>> Is there an option to request the 5.1 mapping?
> >>>
> >>> As noted in the 0/0 cover letter, you can do it just fine with
> >>> `-channel_layout "5.1"` with ffmpeg.c. I have verified that it returns
> >>> the 5.1 channels as-is.
> >>
> >> But this does not make the decoder output 5.1 or does it?
> >>
> >> While I should probably only care about ffmpeg and ignore
> >> the library users I still wonder who can live with your approach...
> >>
> >
> > As this can be implied to be a comment regarding me not caring about
> > the API users,
>
> It was a comment about me not understanding how the average of hundreds of library users will be able to use this - important - new feature.
>
> > and generally being wording that can be interpreted as
> > disagreeing on the whole approach of the patch set, please respond
> > whether this is a blocker or not.
>
> Definitely not and I am sorry if my wording was so bad.
>
> Carl Eugen

Thank you for confirming that this is not a blocker.

Jan
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index 1d6cc36b8c..84fe736454 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,11 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2020-06-16 - xxxxxxxxxx - lavu 56.56.100 - channel_layout.h
+  Add AV_CH_LAYOUT_22POINT2 together with its newly required pieces:
+  AV_CH_TOP_SIDE_LEFT, AV_CH_TOP_SIDE_RIGHT, AV_CH_BOTTOM_FRONT_LEFT,
+  AV_CH_BOTTOM_FRONT_CENTER, AV_CH_BOTTOM_FRONT_RIGHT.
+
 2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
   Add AV_PIX_FMT_X2RGB10.
 
diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
index 3bd5ee29b7..d6bfc74927 100644
--- a/libavutil/channel_layout.c
+++ b/libavutil/channel_layout.c
@@ -62,6 +62,11 @@  static const struct channel_name channel_names[] = {
     [33] = { "SDL",       "surround direct left"  },
     [34] = { "SDR",       "surround direct right" },
     [35] = { "LFE2",      "low frequency 2"       },
+    [36] = { "TSL",       "top side left"         },
+    [37] = { "TSR",       "top side right"        },
+    [38] = { "BFL",       "bottom front left"     },
+    [39] = { "BFC",       "bottom front center"   },
+    [40] = { "BFR",       "bottom front right"    },
 };
 
 static const char *get_channel_name(int channel_id)
@@ -104,6 +109,7 @@  static const struct {
     { "octagonal",   8,  AV_CH_LAYOUT_OCTAGONAL },
     { "hexadecagonal", 16, AV_CH_LAYOUT_HEXADECAGONAL },
     { "downmix",     2,  AV_CH_LAYOUT_STEREO_DOWNMIX, },
+    { "22.2",          24, AV_CH_LAYOUT_22POINT2, },
 };
 
 static uint64_t get_channel_layout_single(const char *name, int name_len)
diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
index 50bb8f03c5..43297505cb 100644
--- a/libavutil/channel_layout.h
+++ b/libavutil/channel_layout.h
@@ -71,6 +71,11 @@ 
 #define AV_CH_SURROUND_DIRECT_LEFT   0x0000000200000000ULL
 #define AV_CH_SURROUND_DIRECT_RIGHT  0x0000000400000000ULL
 #define AV_CH_LOW_FREQUENCY_2        0x0000000800000000ULL
+#define AV_CH_TOP_SIDE_LEFT          0x0000001000000000ULL
+#define AV_CH_TOP_SIDE_RIGHT         0x0000002000000000ULL
+#define AV_CH_BOTTOM_FRONT_LEFT      0x0000004000000000ULL
+#define AV_CH_BOTTOM_FRONT_CENTER    0x0000008000000000ULL
+#define AV_CH_BOTTOM_FRONT_RIGHT     0x0000010000000000ULL
 
 /** Channel mask value used for AVCodecContext.request_channel_layout
     to indicate that the user requests the channel order of the decoder output
@@ -110,6 +115,7 @@ 
 #define AV_CH_LAYOUT_OCTAGONAL         (AV_CH_LAYOUT_5POINT0|AV_CH_BACK_LEFT|AV_CH_BACK_CENTER|AV_CH_BACK_RIGHT)
 #define AV_CH_LAYOUT_HEXADECAGONAL     (AV_CH_LAYOUT_OCTAGONAL|AV_CH_WIDE_LEFT|AV_CH_WIDE_RIGHT|AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_RIGHT|AV_CH_TOP_BACK_CENTER|AV_CH_TOP_FRONT_CENTER|AV_CH_TOP_FRONT_LEFT|AV_CH_TOP_FRONT_RIGHT)
 #define AV_CH_LAYOUT_STEREO_DOWNMIX    (AV_CH_STEREO_LEFT|AV_CH_STEREO_RIGHT)
+#define AV_CH_LAYOUT_22POINT2          (AV_CH_LAYOUT_5POINT1_BACK|AV_CH_FRONT_LEFT_OF_CENTER|AV_CH_FRONT_RIGHT_OF_CENTER|AV_CH_BACK_CENTER|AV_CH_LOW_FREQUENCY_2|AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT|AV_CH_TOP_FRONT_LEFT|AV_CH_TOP_FRONT_RIGHT|AV_CH_TOP_FRONT_CENTER|AV_CH_TOP_CENTER|AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_RIGHT|AV_CH_TOP_SIDE_LEFT|AV_CH_TOP_SIDE_RIGHT|AV_CH_TOP_BACK_CENTER|AV_CH_BOTTOM_FRONT_CENTER|AV_CH_BOTTOM_FRONT_LEFT|AV_CH_BOTTOM_FRONT_RIGHT)
 
 enum AVMatrixEncoding {
     AV_MATRIX_ENCODING_NONE,
diff --git a/libavutil/version.h b/libavutil/version.h
index 3ce9b1831e..a63f79feb1 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  56
-#define LIBAVUTIL_VERSION_MINOR  55
+#define LIBAVUTIL_VERSION_MINOR  56
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \