[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.

[Moritz Barsnick]

Just some small observations... Even if you postpone this patch,
perhaps it can help improve the next one.

On Thu, Jul 06, 2017 at 17:36:39 -0700, Louis O'Bryan wrote:

> @@ -2,6 +2,7 @@ Entries are sorted chronologically from oldest to youngest within each release,
>  releases are sorted from youngest to oldest.
>  
>  version <next>:
> +- Camera metadata motion encoder
>  - deflicker video filter

Did you read that sentence above? ;-) New entries at the bottom of the
"version <next>" section please.

>  @item Zip Motion Blocks Video  @tab   X @tab  X
>      @tab Encoder works only in PAL8.
> + at item Camera metadata motion    @tab X  @tab
> +    @tab Encoder for camera sensor data.

This list is supposed to be alphabetical.

>  OBJS-$(CONFIG_ZLIB_ENCODER)            += lclenc.o
>  OBJS-$(CONFIG_ZMBV_DECODER)            += zmbv.o
>  OBJS-$(CONFIG_ZMBV_ENCODER)            += zmbvenc.o
> +OBJS-$(CONFIG_CAMERA_MOTION_METADATA_ENCODER) += cammenc.o

This also needs to be in alphabetical order.

> + * Reference implementation for the CAMM Metadata encoder.
> + * Encodes sensor data for 360-degree cameras such as
> + * GPS, gyro, and acceleration. This is stored in a track separate from video
> + * and audio.

I believe you're just supposed to have a short entry here, the more
detailed stuff goes into the section below the GPL boilerplate.

> +// 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)
> +};

Doesn't this make the sizes platform dependant? (Unless all platforms
have standardized floats anyway, then I'm mistaken. I just don't know.)

> +            av_log(avctx, AV_LOG_DEBUG,
> +                   "pixel_exposure_time = %lu\n", pixel_exposure_time);

pixel_exposure_time is a uint64_t, so '%lu' is incorrect, it's
'%"PRIu64"'. (Elsewhere as well.) The <stdint.h> types have theitr own
format identifiers.

> +            av_log(avctx, AV_LOG_DEBUG, "rolling_shutter_skew_time = %lu\n",
> +                   rolling_shutter_skew_time);

Ditto.

> +            if (gps_fix_type != 0 && gps_fix_type != 1 && gps_fix_type != 2) {

You could do "gps_fix_type > 2", but since it's sort of an enum, I
guess this is fine.


> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c

This does NOT belong into your patch! (Or only parts of it.) You copied
some modified code over a recent git checkout, thereby apparently
reverting some unrelated recent commits. I recommend you checkout/pull,
modify only you stuff, and then commit locally. That keeps your changes
constrained, even if the code changes upstream.

Please re-apply your changes to a recent checkout.

Moritz