| Message ID | 20240525022813.2292001-2-dev@lynne.ee |
|---|---|
| State | New |
| Headers | show |
| Series | aacdec: add a native xHE-AAC decoder | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Sat, 25 May 2024, Lynne via ffmpeg-devel wrote: > apichanges will be updated upon merging, as well as a version bump. > --- > libavutil/channel_layout.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h > index 8a078d1601..4e19bbbd9e 100644 > --- a/libavutil/channel_layout.h > +++ b/libavutil/channel_layout.h > @@ -79,6 +79,10 @@ enum AVChannel { > AV_CHAN_BOTTOM_FRONT_CENTER, > AV_CHAN_BOTTOM_FRONT_LEFT, > AV_CHAN_BOTTOM_FRONT_RIGHT, > + AV_CHAN_SURROUND_LEFT, > + AV_CHAN_SURROUND_RIGHT, You want to add a channel ID for Surround or Side Surround? Because based on the subsequent AAC patch you want to add it for side surround, but then the AV_CHAN_SURROUND name is confusing, since we are mapping Surround to AV_CHAN_SIDE. So I suggest using AV_CHAN_SIDE_SURROUND_LEFT/RIGHT instead. > + AV_CHAN_TOP_SURROUND_LEFT, > + AV_CHAN_TOP_SURROUND_RIGHT, You will need to extend the channel_names[] array in channel_layout.c with the newly added channel IDs. Regards, Marton
On 25/05/2024 08:10, Marton Balint wrote: > > > On Sat, 25 May 2024, Lynne via ffmpeg-devel wrote: > >> apichanges will be updated upon merging, as well as a version bump. >> --- >> libavutil/channel_layout.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >> index 8a078d1601..4e19bbbd9e 100644 >> --- a/libavutil/channel_layout.h >> +++ b/libavutil/channel_layout.h >> @@ -79,6 +79,10 @@ enum AVChannel { >> AV_CHAN_BOTTOM_FRONT_CENTER, >> AV_CHAN_BOTTOM_FRONT_LEFT, >> AV_CHAN_BOTTOM_FRONT_RIGHT, >> + AV_CHAN_SURROUND_LEFT, >> + AV_CHAN_SURROUND_RIGHT, > > You want to add a channel ID for Surround or Side Surround? Because > based on the subsequent AAC patch you want to add it for side surround, > but then the AV_CHAN_SURROUND name is confusing, since we are mapping > Surround to AV_CHAN_SIDE. So I suggest using > AV_CHAN_SIDE_SURROUND_LEFT/RIGHT instead. > >> + AV_CHAN_TOP_SURROUND_LEFT, >> + AV_CHAN_TOP_SURROUND_RIGHT, > > You will need to extend the channel_names[] array in channel_layout.c > with the newly added channel IDs. Thanks, changed locally. Planning on merging this in 2 days unless there are more comments.
On Sun, 26 May 2024, Lynne via ffmpeg-devel wrote: > On 25/05/2024 08:10, Marton Balint wrote: >> >> >> On Sat, 25 May 2024, Lynne via ffmpeg-devel wrote: >> >>> apichanges will be updated upon merging, as well as a version bump. >>> --- >>> libavutil/channel_layout.h | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >>> index 8a078d1601..4e19bbbd9e 100644 >>> --- a/libavutil/channel_layout.h >>> +++ b/libavutil/channel_layout.h >>> @@ -79,6 +79,10 @@ enum AVChannel { >>> AV_CHAN_BOTTOM_FRONT_CENTER, >>> AV_CHAN_BOTTOM_FRONT_LEFT, >>> AV_CHAN_BOTTOM_FRONT_RIGHT, >>> + AV_CHAN_SURROUND_LEFT, >>> + AV_CHAN_SURROUND_RIGHT, >> >> You want to add a channel ID for Surround or Side Surround? Because based >> on the subsequent AAC patch you want to add it for side surround, but then >> the AV_CHAN_SURROUND name is confusing, since we are mapping Surround to >> AV_CHAN_SIDE. So I suggest using AV_CHAN_SIDE_SURROUND_LEFT/RIGHT instead. >> >>> + AV_CHAN_TOP_SURROUND_LEFT, >>> + AV_CHAN_TOP_SURROUND_RIGHT, >> >> You will need to extend the channel_names[] array in channel_layout.c with >> the newly added channel IDs. > > > Thanks, changed locally. > Planning on merging this in 2 days unless there are more comments. Can you post the updated version of this patch? It is not entirely clear what you added, or e.g. what abbriviation you planning to use for the new channel IDs. Also I noticed one more thing, you also need to add the AV_CH_* variants for the new IDs. 3 days for new API such as this is a bit short, and if your AAC patches depend on it, I suggest you wait a few more days. Thanks, Marton
On 26/05/2024 22:51, Marton Balint wrote: > > > On Sun, 26 May 2024, Lynne via ffmpeg-devel wrote: > >> On 25/05/2024 08:10, Marton Balint wrote: >>> >>> >>> On Sat, 25 May 2024, Lynne via ffmpeg-devel wrote: >>> >>>> apichanges will be updated upon merging, as well as a version bump. >>>> --- >>>> libavutil/channel_layout.h | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >>>> index 8a078d1601..4e19bbbd9e 100644 >>>> --- a/libavutil/channel_layout.h >>>> +++ b/libavutil/channel_layout.h >>>> @@ -79,6 +79,10 @@ enum AVChannel { >>>> AV_CHAN_BOTTOM_FRONT_CENTER, >>>> AV_CHAN_BOTTOM_FRONT_LEFT, >>>> AV_CHAN_BOTTOM_FRONT_RIGHT, >>>> + AV_CHAN_SURROUND_LEFT, >>>> + AV_CHAN_SURROUND_RIGHT, >>> >>> You want to add a channel ID for Surround or Side Surround? Because >>> based >>> on the subsequent AAC patch you want to add it for side surround, >>> but then >>> the AV_CHAN_SURROUND name is confusing, since we are mapping >>> Surround to >>> AV_CHAN_SIDE. So I suggest using AV_CHAN_SIDE_SURROUND_LEFT/RIGHT >>> instead. >>> >>>> + AV_CHAN_TOP_SURROUND_LEFT, >>>> + AV_CHAN_TOP_SURROUND_RIGHT, >>> >>> You will need to extend the channel_names[] array in >>> channel_layout.c with >>> the newly added channel IDs. >> >> >> Thanks, changed locally. >> Planning on merging this in 2 days unless there are more comments. > > Can you post the updated version of this patch? It is not entirely clear > what you added, or e.g. what abbriviation you planning to use for the > new channel IDs. Also I noticed one more thing, you also need to add the > AV_CH_* variants for the new IDs. Sure, posted. I went with exactly what you wrote. > 3 days for new API such as this is a bit short, and if your AAC patches > depend on it, I suggest you wait a few more days. ...its an enum entry. Do you want a design document and a proposal? You could talk to the person who did the research about it, JEEB. Why wait at all? There's only you and JEEB that care about channel layouts, you can review it and give it an LGTM. There's no reason to wait for days, that is not how reviewing is supposed to work. The decoder doesn't depend on it, I have fallback code. I've been waiting for the channel layout situation to be resolved.
Quoting Lynne via ffmpeg-devel (2024-05-26 23:42:41) > ...its an enum entry. Do you want a design document and a proposal? > You could talk to the person who did the research about it, JEEB. > Why wait at all? There's only you and JEEB that care about channel > layouts, you can review it and give it an LGTM. There's no reason to > wait for days, that is not how reviewing is supposed to work. That's exactly how reviewing is supposed to work. Waiting a few days won't kill anyone and allows more people to comment. To the contrary I'm quite unhappy with some recent instances of developers pushing code immediately upon seeing an LGTM, without giving other people the opportunity to look at it.
On 27/05/2024 10:40, Anton Khirnov wrote: > Quoting Lynne via ffmpeg-devel (2024-05-26 23:42:41) >> ...its an enum entry. Do you want a design document and a proposal? >> You could talk to the person who did the research about it, JEEB. >> Why wait at all? There's only you and JEEB that care about channel >> layouts, you can review it and give it an LGTM. There's no reason to >> wait for days, that is not how reviewing is supposed to work. > > That's exactly how reviewing is supposed to work. Waiting a few days > won't kill anyone and allows more people to comment. > > To the contrary I'm quite unhappy with some recent instances of > developers pushing code immediately upon seeing an LGTM, without giving > other people the opportunity to look at it. I'd understand if it was for generic common code, but if its for code that the one pushing it maintains, I don't see a problem with this, this is how it works in pretty much every project out there.
Quoting Lynne via ffmpeg-devel (2024-05-27 10:54:40) > On 27/05/2024 10:40, Anton Khirnov wrote: > > Quoting Lynne via ffmpeg-devel (2024-05-26 23:42:41) > >> ...its an enum entry. Do you want a design document and a proposal? > >> You could talk to the person who did the research about it, JEEB. > >> Why wait at all? There's only you and JEEB that care about channel > >> layouts, you can review it and give it an LGTM. There's no reason to > >> wait for days, that is not how reviewing is supposed to work. > > > > That's exactly how reviewing is supposed to work. Waiting a few days > > won't kill anyone and allows more people to comment. > > > > To the contrary I'm quite unhappy with some recent instances of > > developers pushing code immediately upon seeing an LGTM, without giving > > other people the opportunity to look at it. > > I'd understand if it was for generic common code, but if its for code > that the one pushing it maintains, I don't see a problem with this, this > is how it works in pretty much every project out there. I do see a problem - just because you maintain the code doesn't mean you always understand all its aspects better than everyone, not to mention when reading your own code you tend to see what you meant to write rather than what you actually wrote. There's always a nontrivial possibility that someone could have a meaningful review comment, so why not wait a day or two? There's no harm in it.
On 27/05/2024 11:07, Anton Khirnov wrote: > Quoting Lynne via ffmpeg-devel (2024-05-27 10:54:40) >> On 27/05/2024 10:40, Anton Khirnov wrote: >>> Quoting Lynne via ffmpeg-devel (2024-05-26 23:42:41) >>>> ...its an enum entry. Do you want a design document and a proposal? >>>> You could talk to the person who did the research about it, JEEB. >>>> Why wait at all? There's only you and JEEB that care about channel >>>> layouts, you can review it and give it an LGTM. There's no reason to >>>> wait for days, that is not how reviewing is supposed to work. >>> >>> That's exactly how reviewing is supposed to work. Waiting a few days >>> won't kill anyone and allows more people to comment. >>> >>> To the contrary I'm quite unhappy with some recent instances of >>> developers pushing code immediately upon seeing an LGTM, without giving >>> other people the opportunity to look at it. >> >> I'd understand if it was for generic common code, but if its for code >> that the one pushing it maintains, I don't see a problem with this, this >> is how it works in pretty much every project out there. > > I do see a problem - just because you maintain the code doesn't mean > you always understand all its aspects better than everyone, not to > mention when reading your own code you tend to see what you meant to > write rather than what you actually wrote. There's always a nontrivial > possibility that someone could have a meaningful review comment, so why > not wait a day or two? There's no harm in it. I don't get the point of this discussion, I *am* waiting for this patch, as I wait for most. But in general, it is every maintainer's discretion when they push.
diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h index 8a078d1601..4e19bbbd9e 100644 --- a/libavutil/channel_layout.h +++ b/libavutil/channel_layout.h @@ -79,6 +79,10 @@ enum AVChannel { AV_CHAN_BOTTOM_FRONT_CENTER, AV_CHAN_BOTTOM_FRONT_LEFT, AV_CHAN_BOTTOM_FRONT_RIGHT, + AV_CHAN_SURROUND_LEFT, + AV_CHAN_SURROUND_RIGHT, + AV_CHAN_TOP_SURROUND_LEFT, + AV_CHAN_TOP_SURROUND_RIGHT, /** Channel is empty can be safely skipped. */ AV_CHAN_UNUSED = 0x200,