Message ID | 20170517174712.8625-3-vittorio.giovara@gmail.com |
---|---|
State | New |
Headers | show |
On 5/17/17 7:46 PM, Vittorio Giovara wrote: > + av_channel_layout_uninit(dst); > + return av_channel_layout_copy(dst, channel_layout); Maybe put the uninit directly in the layout_copy so there isn't risk to leak memory if you forget. lu
On Tue, May 23, 2017 at 4:08 PM, Luca Barbato <lu_zero@gentoo.org> wrote: > On 5/17/17 7:46 PM, Vittorio Giovara wrote: >> + av_channel_layout_uninit(dst); >> + return av_channel_layout_copy(dst, channel_layout); > > Maybe put the uninit directly in the layout_copy so there isn't risk to > leak memory if you forget. Added locally, ok otherwise?
On 5/26/17 6:03 PM, Vittorio Giovara wrote: > On Tue, May 23, 2017 at 4:08 PM, Luca Barbato <lu_zero@gentoo.org> wrote: >> On 5/17/17 7:46 PM, Vittorio Giovara wrote: >>> + av_channel_layout_uninit(dst); >>> + return av_channel_layout_copy(dst, channel_layout); >> >> Maybe put the uninit directly in the layout_copy so there isn't risk to >> leak memory if you forget. > > Added locally, ok otherwise? If you also changed av_channel_layout_from_mask, the rest of the set seems fine as well. lu
Quoting Luca Barbato (2017-05-23 22:08:49) > On 5/17/17 7:46 PM, Vittorio Giovara wrote: > > + av_channel_layout_uninit(dst); > > + return av_channel_layout_copy(dst, channel_layout); > > Maybe put the uninit directly in the layout_copy so there isn't risk to > leak memory if you forget. I am against this. Freeing memory should be explicit.
On Fri, 26 May 2017 19:30:40 +0200 Anton Khirnov <anton@khirnov.net> wrote: > Quoting Luca Barbato (2017-05-23 22:08:49) > > On 5/17/17 7:46 PM, Vittorio Giovara wrote: > > > + av_channel_layout_uninit(dst); > > > + return av_channel_layout_copy(dst, channel_layout); > > > > Maybe put the uninit directly in the layout_copy so there isn't risk to > > leak memory if you forget. > > I am against this. Freeing memory should be explicit. > Makes for hard to use API. Like I never know whether AVFrame dest parameters require the frame to be "initialized" or whatever.
Quoting wm4 (2017-05-26 20:03:07) > On Fri, 26 May 2017 19:30:40 +0200 > Anton Khirnov <anton@khirnov.net> wrote: > > > Quoting Luca Barbato (2017-05-23 22:08:49) > > > On 5/17/17 7:46 PM, Vittorio Giovara wrote: > > > > + av_channel_layout_uninit(dst); > > > > + return av_channel_layout_copy(dst, channel_layout); > > > > > > Maybe put the uninit directly in the layout_copy so there isn't risk to > > > leak memory if you forget. > > > > I am against this. Freeing memory should be explicit. > > > > Makes for hard to use API. > > Like I never know whether AVFrame dest parameters require the frame to > be "initialized" or whatever. Hard? I'd agree with "more verbose", but not hard. AVFrame should always be in one of two states: - empty, after it's allocated or unreffed or move_ref()ed from or such - filled, when somebody put something in it When you pass it to any APIs that write into it, it should be empty. I don't see how that is hard, it seems pretty intuitive to me. OTOH implicit frees make it easy to leak stuff, because then it becomes unclear what you do or don't have to free explicitily.
On 5/26/17 7:30 PM, Anton Khirnov wrote: > Quoting Luca Barbato (2017-05-23 22:08:49) >> On 5/17/17 7:46 PM, Vittorio Giovara wrote: >>> + av_channel_layout_uninit(dst); >>> + return av_channel_layout_copy(dst, channel_layout); >> >> Maybe put the uninit directly in the layout_copy so there isn't risk to >> leak memory if you forget. > > I am against this. Freeing memory should be explicit. > Pretty much every use (bar maybe 2) of the two mentioned function needs an uninit in front of it. lu
On Fri, May 26, 2017 at 1:30 PM, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Luca Barbato (2017-05-23 22:08:49) >> On 5/17/17 7:46 PM, Vittorio Giovara wrote: >> > + av_channel_layout_uninit(dst); >> > + return av_channel_layout_copy(dst, channel_layout); >> >> Maybe put the uninit directly in the layout_copy so there isn't risk to >> leak memory if you forget. > > I am against this. Freeing memory should be explicit. Well it can be "explicitly" documented ;) But seriously, it's incredibly easy to forget deallocating before copying, and while I see the reason for not bundling _uninit with _from_mask or _default, I think it makes sense here.
diff --git a/libavutil/opt.c b/libavutil/opt.c index 44d6299117..2a814b5d58 100644 --- a/libavutil/opt.c +++ b/libavutil/opt.c @@ -242,6 +242,14 @@ static int set_string_number(void *obj, void *target_obj, const AVOption *o, con } } +static int set_string_channel_layout(void *obj, const AVOption *o, + const char *val, void *dst) +{ + AVChannelLayout *channel_layout = dst; + av_channel_layout_uninit(channel_layout); + return av_channel_layout_from_string(channel_layout, val); +} + int av_opt_set(void *obj, const char *name, const char *val, int search_flags) { void *dst, *target_obj; @@ -264,6 +272,8 @@ int av_opt_set(void *obj, const char *name, const char *val, int search_flags) case AV_OPT_TYPE_DOUBLE: case AV_OPT_TYPE_RATIONAL: return set_string_number(obj, target_obj, o, val, dst); + case AV_OPT_TYPE_CHANNEL_LAYOUT: + return set_string_channel_layout(obj, o, val, dst); } av_log(obj, AV_LOG_ERROR, "Invalid option type.\n"); @@ -365,6 +375,23 @@ int av_opt_set_dict_val(void *obj, const char *name, const AVDictionary *val, return 0; } +int av_opt_set_channel_layout(void *obj, const char *name, + const AVChannelLayout *channel_layout, + int search_flags) +{ + void *target_obj; + const AVOption *o = av_opt_find2(obj, name, NULL, 0, search_flags, &target_obj); + AVChannelLayout *dst; + + if (!o || !target_obj) + return AVERROR_OPTION_NOT_FOUND; + + dst = (AVChannelLayout*)((uint8_t*)target_obj + o->offset); + + av_channel_layout_uninit(dst); + return av_channel_layout_copy(dst, channel_layout); +} + int av_opt_get(void *obj, const char *name, int search_flags, uint8_t **out_val) { void *dst, *target_obj; @@ -414,6 +441,9 @@ int av_opt_get(void *obj, const char *name, int search_flags, uint8_t **out_val) for (i = 0; i < len; i++) snprintf(*out_val + i * 2, 3, "%02X", bin[i]); return 0; + case AV_OPT_TYPE_CHANNEL_LAYOUT: + *out_val = av_channel_layout_describe(dst); + return *out_val ? 0 : AVERROR(EINVAL); default: return AVERROR(EINVAL); } @@ -499,6 +529,20 @@ int av_opt_get_dict_val(void *obj, const char *name, int search_flags, AVDiction return 0; } +int av_opt_get_channel_layout(void *obj, const char *name, int search_flags, + AVChannelLayout *channel_layout) +{ + void *dst, *target_obj; + const AVOption *o = av_opt_find2(obj, name, NULL, 0, search_flags, &target_obj); + + if (!o || !target_obj) + return AVERROR_OPTION_NOT_FOUND; + + dst = ((uint8_t*)target_obj) + o->offset; + + return av_channel_layout_copy(channel_layout, dst); +} + int av_opt_flag_is_set(void *obj, const char *field_name, const char *flag_name) { const AVOption *field = av_opt_find(obj, field_name, NULL, 0, 0); @@ -561,6 +605,9 @@ static void opt_list(void *obj, void *av_log_obj, const char *unit, case AV_OPT_TYPE_BINARY: av_log(av_log_obj, AV_LOG_INFO, "%-7s ", "<binary>"); break; + case AV_OPT_TYPE_CHANNEL_LAYOUT: + av_log(av_log_obj, AV_LOG_INFO, "%-7s", "<channel layout>"); + break; case AV_OPT_TYPE_CONST: default: av_log(av_log_obj, AV_LOG_INFO, "%-7s ", ""); @@ -626,6 +673,7 @@ void av_opt_set_defaults(void *s) } break; case AV_OPT_TYPE_STRING: + case AV_OPT_TYPE_CHANNEL_LAYOUT: av_opt_set(s, opt->name, opt->default_val.str, 0); break; case AV_OPT_TYPE_BINARY: diff --git a/libavutil/opt.h b/libavutil/opt.h index b68a396da7..a30c53b0ab 100644 --- a/libavutil/opt.h +++ b/libavutil/opt.h @@ -29,6 +29,7 @@ #include "rational.h" #include "avutil.h" +#include "channel_layout.h" #include "dict.h" #include "log.h" @@ -225,6 +226,11 @@ enum AVOptionType{ AV_OPT_TYPE_RATIONAL, AV_OPT_TYPE_BINARY, ///< offset must point to a pointer immediately followed by an int for the length AV_OPT_TYPE_DICT, + /** + * The offset point to an AVChannelLayout, the default is .str, which gets + * passed to av_channel_layout_from_string(). + */ + AV_OPT_TYPE_CHANNEL_LAYOUT, AV_OPT_TYPE_CONST = 128, }; @@ -499,6 +505,8 @@ int av_opt_set_bin (void *obj, const char *name, const uint8_t *val, int siz * caller still owns val is and responsible for freeing it. */ int av_opt_set_dict_val(void *obj, const char *name, const AVDictionary *val, int search_flags); +int av_opt_set_channel_layout(void *obj, const char *name, + const AVChannelLayout *channel_layout, int search_flags); /** * @} */ @@ -528,6 +536,8 @@ int av_opt_get_q (void *obj, const char *name, int search_flags, AVRationa */ int av_opt_get_dict_val(void *obj, const char *name, int search_flags, AVDictionary **out_val); +int av_opt_get_channel_layout(void *obj, const char *name, int search_flags, + AVChannelLayout *channel_layout); /** * Copy options from src object into dest object. *