On Fri, 30 Jun 2017 12:51:16 -0400 Vittorio Giovara <vittorio.giov...@gmail.com> wrote:
> On Fri, Jun 30, 2017 at 4:38 AM, wm4 <nfx...@googlemail.com> wrote: > > On Thu, 29 Jun 2017 17:28:51 -0400 > > Vittorio Giovara <vittorio.giov...@gmail.com> wrote: > > > >> From: Anton Khirnov <an...@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.giov...@gmail.com>. > >> Signed-off-by: Vittorio Giovara <vittorio.giov...@gmail.com> > >> --- > > > > > >> + > >> +/** > >> + * Initialize a channel layout from a given string description. > >> + * The input string can be represented by: > >> + * - the formal channel layout name (returned by > >> av_channel_layout_describe()) > >> + * - single or multiple channel names (returned by av_channel_name() > >> + * or concatenated with "|") > >> + * - a hexadecimal value of a channel layout (eg. "0x4") > >> + * - the number of channels with default layout (eg. "5") > >> + * - the number of unordered channels (eg. "4 channels") > >> + * > >> + * @note channel_layout should be properly allocated as described above. > > > > Above, where? > > in the structure description, I'll add this locally > > >> + * > >> + * @param channel_layout input channel layout > >> + * @param str string describing the channel layout > >> + * @return 0 channel layout was detected, AVERROR_INVALIDATATA otherwise > >> + */ > >> +int av_channel_layout_from_string(AVChannelLayout *channel_layout, > >> + const char *str); > > > > I still think you need to describe in what state channel_layout should > > be. > > > > Why is this so hard? It's always the same with Libav API docs. AVFrame > > sort of has the same problem. > > ok if you insist ^^ > > >> +/** > >> + * Get the default channel layout for a given number of channels. > >> + * > >> + * @note channel_layout should be properly allocated as described above. > >> + * > >> + * @param channel_layout the layout structure to be initialized > >> + * @param nb_channels number of channels > >> + */ > >> +void av_channel_layout_default(AVChannelLayout *ch_layout, int > >> nb_channels); > >> + > >> +/** > >> + * Free any allocated data in the channel layout and reset the channel > >> + * count to 0. > >> + * > >> + * @note this only used for structure initialization and for freeing the > >> + * allocated memory for AV_CHANNEL_ORDER_CUSTOM order. > >> + * > >> + * @param channel_layout the layout structure to be uninitialized > >> + */ > >> +void av_channel_layout_uninit(AVChannelLayout *channel_layout); > >> + > >> +/** > >> + * Make a copy of a channel layout. This differs from just assigning src > >> to dst > >> + * in that it allocates and copies the map for AV_CHANNEL_ORDER_CUSTOM. > >> + * > > > >> + * @note the destination channel_layout will be always uninitialized > >> before copy. > > > > What does this statement even mean? Does it call > > > > av_channel_layout_uninit(dst); > > > > as first line? > > Correct, is that unclear? How would you phrase it? Well, the situation is that memory allocations are involved, and the state of the dst argument is unclear. You have to define exactly in what state it has to be, or the user is inevitably going to make mistakes, unless he reads the source code and makes assumptions about the API based on it. In such a case, there are about the following choices: 1. The function will blindly overwrite dst, and not ever read from it. In this case, the API user doesn't need to care what is in dst as long as he made sure that there are no allocations that would leak. The user can put uninitialized (i.e. random) data into dst before calling the function. 2. The function explicitly uninitializes the dst argument before it writes to it. The API user must make sure that dst is somehow valid. I suppose at least in this case, both passing zerod-out memory, or something created with a constructor function is fine. 3. Something in-between, like requiring zerod-out memory, and having undefined behavior if it's a valid allocation (yes there are functions like this in Libav). Your implementation is 2., but you write something about how it's "uninitialized" (now that I read it again, I think it says in English that av_channel_layout_uninit(dst); is called). The docs should be as explicit as possible here. Even better if we had (or have?) some sort of consistent standard terminology for these things. And maybe consistent API behavior. I for one tend to have to look up which behavior a given function expects. _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
