[002/200] lavu: support AVChannelLayout AVOptions

Message ID 20170517174712.8625-3-vittorio.giovara@gmail.com
State New
Headers show

Commit Message

Vittorio Giovara May 17, 2017, 5:46 p.m.
From: Anton Khirnov <anton@khirnov.net>

Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
---
 libavutil/opt.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 libavutil/opt.h | 10 ++++++++++
 2 files changed, 58 insertions(+)

Comments

Luca Barbato May 23, 2017, 8:08 p.m. | #1
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
Vittorio Giovara May 26, 2017, 4:03 p.m. | #2
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?
Luca Barbato May 26, 2017, 4:37 p.m. | #3
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
Anton Khirnov May 26, 2017, 5:30 p.m. | #4
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.
wm4 May 26, 2017, 6:03 p.m. | #5
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.
Anton Khirnov May 26, 2017, 6:12 p.m. | #6
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.
Luca Barbato May 26, 2017, 8:22 p.m. | #7
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
Vittorio Giovara May 30, 2017, 3:05 p.m. | #8
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.

Patch

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.
  *