[FFmpeg-devel] [PATCH v8 09/13] avfilter/textmod: Add textmod, censor and show_speaker filters

[Andreas Rheinhardt]

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 05:08
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v8 09/13] avfilter/textmod: Add textmod,
>> censor and show_speaker filters
>>
> 
> [..]
>  
>> This function is horrible (and I already wanted to deprecate it, but for
>> this I would need to fix all current users of it): It automatically
>> frees the array on error and it returns void, thereby encouraging a
>> programming style in which errors are not checked. (You are checking
>> them.) The automatic free on error means that there are basically only
>> two usecases for it (unless one just ignores any errors): To store
>> pointers to objects that don't need to be freed at all (like static
>> objects) or to store non-ownership pointers, like pointers to substrings
>> of a string. Now that you started strduping the strings you are no
>> longer of this type. In other words, there will be leaks on error.
>>
>> Anyway, I don't see why you went the route of using individually
>> allocated strings.
> 
> [..]
> 
>> Leak on error, as above.
>> (Notice that you do not need to allocate the replace_list entries
>> individually; all you need to do is strdup s->find before it is split
>> into little pieces. Then you can get the offset of the nth replace
>> string in the duplicate of s->find by s->find_list[n] - s->find.)
>> Finally, when dealing with the replace list you already know in advance
>> how many entries there will be, so you don't have to reallocate.
> 
> The reason why I changed to individual allocation is that eventually 
> I want to be able to:
> 
> - Split by multiple separators, e.g. '\r', '\n' in addition to the 
>   chosen separator
> - Remove empty entries (when two separators follow after each other)
> - Trim leading and trailing whitespace from each entry
> 
> Doing all these things with pointers to the full string seems to be 
> too much of a hazzle and error prone.
> 

1. av_strtok() can separate by multiple separators: The deliminator is
actually supposed to be a NUL-terminated string. In other words: Using a
pointer to a char is wrong and may crash.
2. av_strtok() already trims empty entries. (And if it did not, all you
needed to do were to check for emptiness before adding the pointer to
the list, like you already do.)
3. The last thing is also easily achievable with non-allocated pointers.

As long as you don't want to enlarge a substring, there is no need for
allocations.

- Andreas