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

[Lynne via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2344 bytes --]

On 23/06/2024 13:05, Derek Buitenhuis wrote:
> 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.

Sorry. I somehow missed this patchset, both when it was sent for review 
and when it was merged.
I did mention that I don't have strong feelings about changing this, it 
is just being pedantic, so I can drop it if you'd prefer. There's no 
need to argue over thousands of a degree here.

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

Fair enough, dropped.

> 
>>       /**
>> -     * 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.

Thanks, changed, and set a v2.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".