[FFmpeg-devel] [PATCH 1/1] This change adds an encoder for Camera metadata motion. This is a type of sensor data associated with video, such as GPS, acceleration, gyro, and camera orientation. It does not encode video itself, but rather, this metadata.

Reimar Döffinger Reimar.Doeffinger at gmx.de
Sat Jul 8 01:12:24 EEST 2017


I don't know what you are trying to achieve, but an encoder that just copies things through makes no sense.
If it's just about validating or logging some kind of parser or (bitstream-)filter probably makes more sense.
Even if doing it as encoder, why should it be a video encoder instead of something like raw (ok, not sure we support that) or subtitle?

> +// Sizes of each type of metadata.
> +static int metadata_type_sizes[] = {
> +  3 * sizeof(float),
> +  2 * sizeof(uint64_t),
> +  3 * sizeof(float),
> +  3 * sizeof(float),
> +  3 * sizeof(float),
> +  3 * sizeof(float),
> +  3 * sizeof(double) + sizeof(uint32_t) + 7 * sizeof(float),
> +  3 * sizeof(float)
> +};
> +
> +static int min_packet_size = sizeof(uint16_t) * 2;

These miss "const".
And as someone else mentioned, all the data reading needs to use special functions.
If it's still the same as when I was more active, that includes the float values.
Not all platforms use IEEE floats and there used to be platforms where float values had an endiannes different from the integers etc.
You should probably also consider if you validate against things like M_PI / 4 how this deals with rounding errors.
If you had a spec, you would probably have the exact bit pattern of the minimum and maximum and you should then do a total order comparison against that, preferably without even converting to double.

> +static int validate_latitude(AVCodecContext *avctx, double latitude) {
> +    if (latitude < -M_PI / 4 || latitude > M_PI / 4) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "Latitude %f is not in bounds (%f, %f)\n",
> +               latitude, -M_PI / 4, M_PI / 4);
> +        return 0;
> +    }

This one in particular will allow all NaNs as valid, which I suspect is not the intention, and if it is should be made more explicit.


More information about the ffmpeg-devel mailing list