[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 barsnick at gmx.net
Fri Jul 7 13:14:45 EEST 2017


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


More information about the ffmpeg-devel mailing list