Re: [FFmpeg-devel] [PATCH 001/293 v7] Add a new channel layout API

[James Almer <jamrial@gmail.com>]

On 1/23/2022 8:32 PM, Marton Balint wrote:
> 
> 
> On Thu, 20 Jan 2022, James Almer wrote:
> 
>> From: Anton Khirnov <anton@khirnov.net>
>>
>> The new API is more extensible and allows for custom layouts.
>> More accurate information is exported, eg for decoders that do not
>> set a channel layout, lavc will not make one up for them.
>>
>> Deprecate the old API working with just uint64_t bitmasks.
>>
>> Expanded and completed by Vittorio Giovara <vittorio.giovara@gmail.com>
>> and James Almer <jamrial@gmail.com>.
>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Changes since last version:
>>
>> *Implemented Marton's suggestion for 
>> av_channel_layout_index_from_string()
>> and av_channel_layout_channel_from_string(), to not mix designation and
>> custom name namespaces.
> 
> Thanks.
> 
> [...]
> 
>> +enum AVChannel
>> +av_channel_layout_channel_from_string(const AVChannelLayout 
>> *channel_layout,
>> +                                      const char *str)
>> +{
> 
> As far as I see this function equvivalent to
> 
>      int index = av_channel_layout_index_from_string(channel_layout, str);
>      if (index < 0)
>          return AV_CHAN_NONE;
>      return av_channel_layout_channel_from_index(channel_layout, index);
> 
> This would avoid code duplication.

Funny how i even mentioned this much but then duplicated all this code...

> 
> [...]
> 
>> +int av_channel_layout_index_from_string(const AVChannelLayout 
>> *channel_layout,
>> +                                        const char *str)
>> +{
>> +    char *chname, buf[16];
>> +    enum AVChannel ch = AV_CHAN_NONE;
>> +
>> +    switch (channel_layout->order) {
>> +    case AV_CHANNEL_ORDER_CUSTOM:
>> +        chname = strstr(str, "@");
>> +        if (chname) {
>> +            char buf[16];
>> +            chname++;
>> +            av_strlcpy(buf, str, FFMIN(sizeof(buf), chname - str));
>> +            if (!*chname)
>> +                chname = NULL;
>> +            ch = av_channel_from_string(buf);
> 
> If designation is present but invalid we should return error, e.g:
> 
>                 if (ch == AV_CHAN_NONE && *buf)
>                     return AVERROR(EINVAL);

Good catch, will fix.

> 
> [...]
> 
>> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
>> index d39ae1177a..2efac9046a 100644
>> --- a/libavutil/channel_layout.h
>> +++ b/libavutil/channel_layout.h
>> @@ -23,6 +23,10 @@
>> #define AVUTIL_CHANNEL_LAYOUT_H
>>
>> #include <stdint.h>
>> +#include <stdlib.h>
>> +
>> +#include "version.h"
>> +#include "attributes.h"
>>
>> /**
>>  * @file
>> @@ -34,6 +38,71 @@
>>  * @{
>>  */
>>
>> +enum AVChannel {
>> +    ///< Invalid channel index
>> +    AV_CHAN_NONE = -1,
>> +    AV_CHAN_FRONT_LEFT,
>> +    AV_CHAN_FRONT_RIGHT,
>> +    AV_CHAN_FRONT_CENTER,
>> +    AV_CHAN_LOW_FREQUENCY,
>> +    AV_CHAN_BACK_LEFT,
>> +    AV_CHAN_BACK_RIGHT,
>> +    AV_CHAN_FRONT_LEFT_OF_CENTER,
>> +    AV_CHAN_FRONT_RIGHT_OF_CENTER,
>> +    AV_CHAN_BACK_CENTER,
>> +    AV_CHAN_SIDE_LEFT,
>> +    AV_CHAN_SIDE_RIGHT,
>> +    AV_CHAN_TOP_CENTER,
>> +    AV_CHAN_TOP_FRONT_LEFT,
>> +    AV_CHAN_TOP_FRONT_CENTER,
>> +    AV_CHAN_TOP_FRONT_RIGHT,
>> +    AV_CHAN_TOP_BACK_LEFT,
>> +    AV_CHAN_TOP_BACK_CENTER,
>> +    AV_CHAN_TOP_BACK_RIGHT,
>> +    /** Stereo downmix. */
>> +    AV_CHAN_STEREO_LEFT = 29,
>> +    /** See above. */
>> +    AV_CHAN_STEREO_RIGHT,
>> +    AV_CHAN_WIDE_LEFT,
>> +    AV_CHAN_WIDE_RIGHT,
>> +    AV_CHAN_SURROUND_DIRECT_LEFT,
>> +    AV_CHAN_SURROUND_DIRECT_RIGHT,
>> +    AV_CHAN_LOW_FREQUENCY_2,
>> +    AV_CHAN_TOP_SIDE_LEFT,
>> +    AV_CHAN_TOP_SIDE_RIGHT,
>> +    AV_CHAN_BOTTOM_FRONT_CENTER,
>> +    AV_CHAN_BOTTOM_FRONT_LEFT,
>> +    AV_CHAN_BOTTOM_FRONT_RIGHT,
>> +
>> +    /** Channel is empty can be safely skipped. */
>> +    AV_CHAN_UNUSED = 64,
>> +
>> +    /** Channel contains data, but its position is unknown. */
>> +    AV_CHAN_UNKWNOWN = 128,
> 
> typo: UNKNOWN

Will fix.

> 
> No more comments for me, thanks.
> 
> Marton
> _______________________________________________
> 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".
_______________________________________________
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".