[FFmpeg-cvslog] r22119 - in trunk/libavutil: tree.c tree.h

[Vitor Sessak]

Michael Niedermayer wrote:
> On Tue, Mar 02, 2010 at 07:18:09PM +0100, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Tue, Mar 02, 2010 at 06:40:03AM +0100, Vitor Sessak wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Sun, Feb 28, 2010 at 09:48:42PM +0100, vitor wrote:
>>>>>> Author: vitor
>>>>>> Date: Sun Feb 28 21:48:42 2010
>>>>>> New Revision: 22119
>>>>>>
>>>>>> Log:
>>>>>> Implement av_tree_destroy_free_elem() to destroy a tree and free all 
>>>>>> the values stored on it.
>>>>>>
>>>>>> Modified:
>>>>>>    trunk/libavutil/tree.c
>>>>>>    trunk/libavutil/tree.h
>>>>>>
>>>>>> Modified: trunk/libavutil/tree.c
>>>>>> ==============================================================================
>>>>>> --- trunk/libavutil/tree.c	Sun Feb 28 20:58:26 2010	(r22118)
>>>>>> +++ trunk/libavutil/tree.c	Sun Feb 28 21:48:42 2010	(r22119)
>>>>>> @@ -135,6 +135,15 @@ void av_tree_destroy(AVTreeNode *t){
>>>>>>      }
>>>>>>  }
>>>>>>  +void av_tree_destroy_free_elem(AVTreeNode *t){
>>>>>> +    if(t){
>>>>>> +        av_tree_destroy_free_elem(t->child[0]);
>>>>>> +        av_tree_destroy_free_elem(t->child[1]);
>>>>>> +        av_free(t->elem);
>>>>>> +        av_free(t);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>  #if 0
>>>>>>  void av_tree_enumerate(AVTreeNode *t, void *opaque, int (*cmp)(void 
>>>>>> *opaque, void *elem), int (*enu)(void *opaque, void *elem)){
>>>>>>      if(t){
>>>>>>
>>>>>> Modified: trunk/libavutil/tree.h
>>>>>> ==============================================================================
>>>>>> --- trunk/libavutil/tree.h	Sun Feb 28 20:58:26 2010	(r22118)
>>>>>> +++ trunk/libavutil/tree.h	Sun Feb 28 21:48:42 2010	(r22119)
>>>>>> @@ -78,5 +78,6 @@ void *av_tree_find(const struct AVTreeNo
>>>>>>   */
>>>>>>  void *av_tree_insert(struct AVTreeNode **rootp, void *key, int 
>>>>>> (*cmp)(void *key, const void *b), struct AVTreeNode **next);
>>>>>>  void av_tree_destroy(struct AVTreeNode *t);
>>>>>> +void av_tree_destroy_free_elem(struct AVTreeNode *t);
>>>>> argh, ive missed this part of your patch
>>>>> minor api bump & doxy missing and its the wrong way to do it
>>>>> you dont know at all if avfree() is correct and sufficient to free the
>>>>> opaque elements.
>>>>> av_tree_enumerate() is likely the correct way to free
>>>> Ok, but current version of av_tree_enumerate() is commented out and does 
>>>> not do what one would expect. What do you think of this patch?
>>>>
>>>> -Vitor
>>>>  libavformat/nut.c    |   11 +++++++++++
>>>>  libavformat/nut.h    |    1 +
>>>>  libavformat/nutdec.c |    1 +
>>>>  libavformat/nutenc.c |    1 +
>>>>  libavutil/avutil.h   |    2 +-
>>>>  libavutil/tree.c     |   14 ++++++--------
>>>>  libavutil/tree.h     |    7 +++++++
>>>>  7 files changed, 28 insertions(+), 9 deletions(-)
>>>> 073f43adb2344cede149943eef2ce2cae50d7930  nuttree.diff
>>>> Index: libavutil/tree.c
>>>> ===================================================================
>>>> --- libavutil/tree.c	(revision 22135)
>>>> +++ libavutil/tree.c	(working copy)
>>>> @@ -135,16 +135,14 @@
>>>>      }
>>>>  }
>>>>  -#if 0
>>>> -void av_tree_enumerate(AVTreeNode *t, void *opaque, int (*cmp)(void 
>>>> *opaque, void *elem), int (*enu)(void *opaque, void *elem)){
>>>> -    if(t){
>>>> -        int v= cmp ? cmp(opaque, t->elem) : 0;
>>>> -        if(v>=0) av_tree_enumerate(t->child[0], opaque, cmp, enu);
>>>> -        if(v==0) enu(opaque, t->elem);
>>>> -        if(v<=0) av_tree_enumerate(t->child[1], opaque, cmp, enu);
>>>> +void av_tree_enumerate(AVTreeNode *t, void *opaque,
>>>> +                       void (*enu)(void *opaque, void **elem)){
>>>> +    if (t) {
>>>> +        av_tree_enumerate(t->child[0], opaque, enu);
>>>> +        enu(opaque, &t->elem);
>>>> +        av_tree_enumerate(t->child[1], opaque, enu);
>>>>      }
>>> no, this is broken
>>> the only question on the API i had IIRC was if we should allow elements
>>> to be removed based on the return code of enu()
>>> ruining the API by droping 2/3 of the features is not an option
>> Ok, so since there is no documentation for it, can you please explain me 
>> what av_tree_enumerate() is supposed to do and in what way it is different 
>> from
>>
>> {
>>     void *elem  = av_tree_find(t, opaque, cmp, NULL);
>>     if (elem)
>>         enu(opaque, elem);
>> }
> 
> av_tree_enumerate() is able to enumerate() ranges not just individual
> elements
> cmp returns <0 or >0 -> tested element is outside the range to the specified
> side, otherwise cmp=0 element is inside the range
> 
> suggestions for improvments are welcome

Ok, thanks. Attached patch fixes the function and use it to plug the 
memleak.

-Vitor