[FFmpeg-devel] [PATCH] lavu/stereo3d: change baseline and horizontal FOV to rationals

[Derek Buitenhuis]

On 6/22/2024 9:11 PM, Lynne via ffmpeg-devel wrote:
> The API was committed 2 days ago, so changing these fields now
> is within the realms of acceptable.
> ---

You also had weeks to comment on 3 revisions of the set, and it would
have been nice to hear about this then instead of changing it after I've
already pushed and integrated it in my own downstream work.

If I sound grumpy, it's because I am... the set sat commentless. 

> diff --git a/libavutil/stereo3d.h b/libavutil/stereo3d.h
> index 00a5c3900e..097f6c9455 100644
> --- a/libavutil/stereo3d.h
> +++ b/libavutil/stereo3d.h
> @@ -213,9 +213,9 @@ typedef struct AVStereo3D {
>  
>      /**
>       * The distance between the centres of the lenses of the camera system,
> -     * in micrometers. Zero if unset.
> +     * in meters. Zero if unset.
>       */
> -    uint32_t baseline;
> +    AVRational baseline;

Can't be AVRational for the reason James mentioned, uint32_t can't be put in an int,
and can't be negative.

Also, in terms of units, using meters here is silly, it's the distance between two
lenses, meters is an crazy unit to measure that in, not idiomatic at all, and I
would argue violates the principal of least surprise (both in terms of units, and
being a distance as a rational).

>      /**
> -     * Horizontal field of view in thousanths of a degree. Zero if unset.
> +     * Horizontal field of view, in degrees. Zero if unset.
>       */
> -    uint32_t horizontal_field_of_view;
> +    AVRational horizontal_field_of_view;
>  } AVStereo3D;

This one is more reasonable. I am not happy about the post-push change, but
if the community wants this one, I am not going to block it.

> +horizontal_field_of_view=0/0

If it is changed, this should be initialized to 0/1, as that is what James
last set did for the rest of all side data.

- Derek