[FFmpeg-devel] [PATCHv4] lavf: add libopenmpt demuxer

[Jörn Heusipp]

On 06/30/2016 02:49 AM, Josh de Kock wrote:
> Fixes ticket #5623
>
> TODO: bump lavf minor
> ---

I looked more thoroughly at your patch again.
I'm commenting from libopenmpt perspective of course.


> +    openmpt->module = openmpt_module_create_from_memory(buf, size, openmpt_logfunc, s, NULL);
> +    av_freep(&buf);
> +    if (!openmpt->module)
> +            return AVERROR(ENOMEM);

Libopenmpt C API sadly does not allow to determine the reason for a 
failing openmpt_module_create. This was an oversight when designing the 
API. It will be improved with the 1.0 API which is currently in 
development. We will continue to support the API (and ABI) of 0.2 just 
fine even after 1.0.
As to the specific error code: ENOMEM is of course a possible reason why 
openmpt_module_create_from_memory could fail, but a very unlikely one. 
The more common case would be an invalid/corrupt/unsupported module file.
I think, replacing AVERROR(ENOMEM) with something resembling that would 
be better.


> +#define add_meta(s, name, meta)                    \
> +do {                                               \
> +    const char *value = meta;                      \
> +    if (value && value[0])                         \
> +        av_dict_set(&s->metadata, name, value, 0); \
> +} while(0)
> +
> +static int read_header_openmpt(AVFormatContext *s)
> +{
 > [...]
> +    add_meta(s, "artist",  openmpt_module_get_metadata(openmpt->module, "artist"));
> +    add_meta(s, "title",   openmpt_module_get_metadata(openmpt->module, "title"));
> +    add_meta(s, "encoder", openmpt_module_get_metadata(openmpt->module, "tracker"));
> +    add_meta(s, "comment", openmpt_module_get_metadata(openmpt->module, "message"));

This will leak memory.
All strings returned by libopenmpt are dynamically allocated and need to 
be freed with openmpt_free_string() after use.
See https://lib.openmpt.org/doc/libopenmpt_c_overview.html and 
https://lib.openmpt.org/doc/group__libopenmpt__c.html#ga0fb70a57725fc29499c557974f16a873 
.
Looking at the av_dict_set API, I can see one might be tempted to just 
pass AV_DICT_DONT_STRDUP_VAL in order to resolve this. Please don't, as 
this will break on Windows if libavformat links dynamically to 
libopenmpt which may itself be linked statically against the MSVC 
runtime or dynamically against a different instance of the runtime as 
ffmpeg is using. I do not know whether ffmpeg wants to support this use 
case, but libopenmpt (C API only) is intended to be usable this way, 
which is why one must use openmpt_free_string() which will call the 
appropriate free() function internally.

i.e. something like:
#define add_meta_and_free(s, name, meta)           \
do {                                               \
     const char *value = meta;                      \
     if (value && value[0])                         \
         av_dict_set(&s->metadata, name, value, 0); \
     openmpt_free_string(value);                    \
} while(0)


Regards,
Jörn (manx on htts://bugs.openmpt.org/)