[FFmpeg-devel] [PATCH] lavu/stereo3d: change baseline and horizontal FOV to rationals
Lynne
dev at lynne.ee
Sun Jun 23 01:07:09 EEST 2024
On 22/06/2024 23:30, James Almer wrote:
> On 6/22/2024 6:26 PM, Lynne via ffmpeg-devel wrote:
>> On 22/06/2024 22:19, James Almer wrote:
>>> 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.
>>
>> Added an (int32_t) cast locally. This is also how
>> horizontal_disparity_adjustment is handled, with a uint32_t converted
>> into a rational.
>
> No, that one is an int32_t as it can be negative (Ranges -1.0 to 1.0),
> thus it works fine as AVRational. baseline does not, so it should stay
> an uint32_t.
I doubt that anyone will use a baseline greater than 2.1 (2^31/2*10^6)
kilometers, but sure, this part is dropped. I more strongly doubt anyone
will use a baseline of less than a micrometer, mechanics start to play
with dice, and atomic force microscopy has coarser resolution than this.
>>
>>>
>>> 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?
>>
>> Likely a float (EBML_FLOAT). This is how mastering metadata and video
>> projection are handled.
>
> But Mastering metadata is not in degrees.
We use rationals for everything, not just framerates and ratios.
> Wait for Derek to comment. He knows this field better than me.
Yup, that's the plan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xA2FEA5F03F034464.asc
Type: application/pgp-keys
Size: 624 bytes
Desc: OpenPGP public key
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240623/7a37a035/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240623/7a37a035/attachment.sig>
More information about the ffmpeg-devel
mailing list