Quoting wm4 (2017-07-05 20:53:54) > 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 fully agree with you about consistent API behaviour, but the one that's consistent across Libav is with no implicit frees. So 1. from your list. -- Anton Khirnov _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
