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

[James Almer]

On 6/22/2024 5:11 PM, Lynne via ffmpeg-devel wrote:
> This avoids hardcoding any implementation-specific limitiations as
> part of the API, and allows for future expandability.
> 
> This also allows API users to more conveniently convert the
> values into floats without hardcoding specific conversion constants.
> 
> The draft implementation of this mechanism for the draft of
> AVTransport uses rationals for these particular fields.
> 
> The API was committed 2 days ago, so changing these fields now
> is within the realms of acceptable.
> ---
>   fftools/ffprobe.c                            |  4 ++--
>   libavformat/dump.c                           |  9 +++++----
>   libavformat/mov.c                            |  9 ++++++---
>   libavutil/stereo3d.h                         |  8 ++++----
>   tests/ref/fate/matroska-spherical-mono       |  4 ++--
>   tests/ref/fate/matroska-spherical-mono-remux |  8 ++++----
>   tests/ref/fate/matroska-stereo_mode          | 16 ++++++++--------
>   tests/ref/fate/matroska-vp8-alpha-remux      |  4 ++--
>   tests/ref/fate/mov-spherical-mono            |  4 ++--
>   9 files changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index d7ba980ff9..2da928b7c3 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -2546,9 +2546,9 @@ static void print_pkt_side_data(WriterContext *w,
>               print_int("inverted", !!(stereo->flags & AV_STEREO3D_FLAG_INVERT));
>               print_str("view", av_stereo3d_view_name(stereo->view));
>               print_str("primary_eye", av_stereo3d_primary_eye_name(stereo->primary_eye));
> -            print_int("baseline", stereo->baseline);
> +            print_q("baseline", stereo->baseline, '/');
>               print_q("horizontal_disparity_adjustment", stereo->horizontal_disparity_adjustment, '/');
> -            print_int("horizontal_field_of_view", stereo->horizontal_field_of_view);
> +            print_q("horizontal_field_of_view", stereo->horizontal_field_of_view, '/');
>           } else if (sd->type == AV_PKT_DATA_SPHERICAL) {
>               const AVSphericalMapping *spherical = (AVSphericalMapping *)sd->data;
>               print_str("projection", av_spherical_projection_name(spherical->projection));
> diff --git a/libavformat/dump.c b/libavformat/dump.c
> index 61a2c6a29f..ac7d3038ab 100644
> --- a/libavformat/dump.c
> +++ b/libavformat/dump.c
> @@ -262,13 +262,14 @@ static void dump_stereo3d(void *ctx, const AVPacketSideData *sd, int log_level)
>       av_log(ctx, log_level, "%s, view: %s, primary eye: %s",
>              av_stereo3d_type_name(stereo->type), av_stereo3d_view_name(stereo->view),
>              av_stereo3d_primary_eye_name(stereo->primary_eye));
> -    if (stereo->baseline)
> -        av_log(ctx, log_level, ", baseline: %"PRIu32"", stereo->baseline);
> +    if (stereo->baseline.num && stereo->baseline.den)
> +        av_log(ctx, log_level, ", baseline: %d/%d", stereo->baseline.num, stereo->baseline.den);
>       if (stereo->horizontal_disparity_adjustment.num && stereo->horizontal_disparity_adjustment.den)
>           av_log(ctx, log_level, ", horizontal_disparity_adjustment: %d/%d",
>                  stereo->horizontal_disparity_adjustment.num, stereo->horizontal_disparity_adjustment.den);
> -    if (stereo->horizontal_field_of_view)
> -        av_log(ctx, log_level, ", horizontal_field_of_view: %"PRIu32"", stereo->horizontal_field_of_view);
> +    if (stereo->horizontal_field_of_view.num && stereo->horizontal_field_of_view.den)
> +        av_log(ctx, log_level, ", horizontal_field_of_view: %d/%d", stereo->horizontal_field_of_view.num,
> +               stereo->horizontal_field_of_view.den);
>   
>       if (stereo->flags & AV_STEREO3D_FLAG_INVERT)
>           av_log(ctx, log_level, " (inverted)");
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index f08fec3fb6..64cd17e770 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -6545,7 +6545,8 @@ static int mov_read_eyes(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>       MOVStreamContext *sc;
>       int size, flags = 0;
>       int64_t remaining;
> -    uint32_t tag, baseline = 0;
> +    uint32_t tag;
> +    AVRational baseline = av_make_q(0, 0);
>       enum AVStereo3DView view = AV_STEREO3D_VIEW_PACKED;
>       enum AVStereo3DPrimaryEye primary_eye = AV_PRIMARY_EYE_NONE;
>       AVRational horizontal_disparity_adjustment = { 0, 1 };
> @@ -6642,7 +6643,8 @@ static int mov_read_eyes(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>               avio_skip(pb, 1); // version
>               avio_skip(pb, 3); // flags
>   
> -            baseline = avio_rb32(pb);
> +            baseline.num = avio_rb32(pb);
> +            baseline.den = 1000000; // micrometers
>   
>               break;
>           }
> @@ -6782,7 +6784,8 @@ static int mov_read_hfov(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>               return AVERROR(ENOMEM);
>       }
>   
> -    sc->stereo3d->horizontal_field_of_view = avio_rb32(pb);
> +    sc->stereo3d->horizontal_field_of_view.num = avio_rb32(pb);
> +    sc->stereo3d->horizontal_field_of_view.den = 1000; // thousands of a degree
>   
>       return 0;
>   }
> 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;

This is not ok. No reason for distance be a soft float, or allow 
negative values. In the case of the blin box, it's a 32bit unsigned 
value, which will not fit in AVRational.

You can easily convert this in your code by using a 1000000 denominator 
if you wish.

>   
>       /**
>        * Relative shift of the left and right images, which changes the zero parallax plane.
> @@ -224,9 +224,9 @@ typedef struct AVStereo3D {
>       AVRational horizontal_disparity_adjustment;
>   
>       /**
> -     * 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;

This one i can't say. Would for example Matroska implement degrees as a 
float, or uint?