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/)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
