Skip to content

Feature/available image bounds#60

Merged
jminor merged 7 commits intoOpenTimelineIO:mainfrom
Synopsis:feature/available_image_bounds
Apr 21, 2025
Merged

Feature/available image bounds#60
jminor merged 7 commits intoOpenTimelineIO:mainfrom
Synopsis:feature/available_image_bounds

Conversation

@vade
Copy link
Copy Markdown
Collaborator

@vade vade commented Sep 26, 2024

Link the Issue(s) this Pull Request is related to.

Fixes #51

Summarize your change.

I wanted to open a discussion on the 'right' way of exposing in a cross platform swift native manner the image_bounds of a media reference. This is absolutely a draft and just meant to spark some discussion.

We need a standard way to bridge C++ std::optional<IMATH_NAMESPACE::Box2d> across Obj-C and into Swift.

This PR proposes exposes CGRect (see discussion) which is available cross platform in Foundation.

We then add 3 new functions to opentimelineio.h / .m :

bool media_reference_available_image_bounds(CxxRetainer* self, CGRect* );
void media_reference_set_available_image_bounds(CxxRetainer* self, CGRect image_bounds);
void media_reference_clear_available_image_bounds(CxxRetainer* self);

which we can expose to swift MediaReference class as a property which is a CGRect? from the standard library.

Thoughts?

Reference associated tests.

Added 1 test and 1 OTIO file borrowed from the main repo to facilitate testing

@vade vade marked this pull request as draft September 26, 2024 15:33
@vade
Copy link
Copy Markdown
Collaborator Author

vade commented Sep 26, 2024

I guess the question really becomes, do we consider it a requirement to expose the Box data type and its functions as first class citizens in swift?

If the answer is yes, theres work to be done to make the interface and expose it. If not, something like this might be sufficient.

@jchen9
Copy link
Copy Markdown
Contributor

jchen9 commented Sep 26, 2024

As a Swift/Apple platform OTIO user, I would probably prefer having CGRect-based APIs to work with.

CGRect-related data structures might have already been available in Swift Foundation in Linux runtime. I would need to double-check that though.

@vade
Copy link
Copy Markdown
Collaborator Author

vade commented Sep 26, 2024

hey @jchen9 I concur - if they are available, lets def use them. I didn't see them in the Swift Standard Library but I may have missed something!

@jchen9
Copy link
Copy Markdown
Contributor

jchen9 commented Sep 26, 2024

Yeah, they are there. I tried Swift 5.10 and 6.0.1 Swift Linux runtimes.

% uname -a
Linux jchen9 x86_64 #1 SMP PREEMPT_DYNAMIC Thu Jul 25 19:24:21 PDT 2024 x86_64 x86_64 x86_64 GNU/Linux
(jchen9) /home/jchen/Applications/Swift/swift-5.10-DEVELOPMENT-SNAPSHOT-2024-05-31-a-ubi9/usr/bin (jchen) :
% ./swift repl
error: This version of LLDB does not support DWARF version 5 or later.
warning: (x86_64) /lib64/libstdc++.so.6 No LZMA support found for reading .gnu_debugdata section
warning: (x86_64) /lib64/libm.so.6 No LZMA support found for reading .gnu_debugdata section
warning: (x86_64) /lib64/libgcc_s.so.1 No LZMA support found for reading .gnu_debugdata section
Welcome to Swift version 5.10-dev (LLVM e98989b1092ff3a, Swift 0e5cb27c42ec3c7).
Type :help for assistance.
  1> import Foundation
  2> let foo: CGRect = CGRect(
Available completions:
        CGRect( -- from: any Decoder
        CGRect( -- origin: CGPoint, size: CGSize
        CGRect( -- x: CGFloat, y: CGFloat, width: CGFloat, height: CGFloat
        CGRect( -- x: Double, y: Double, width: Double, height: Double
        CGRect( -- x: Int, y: Int, width: Int, height: Int
  2> let foo: CGRect = CGRect(x: 1, y: 2, width: 3, height: 4)
foo: Foundation.CGRect = {
  origin = {
    x = 1
    y = 2
  }
  size = {
    width = 3
    height = 4
  }
}
  3> foo.
Available completions:
        foo. -- contains(rect2: CGRect) -> Bool
        foo. -- contains(point: CGPoint) -> Bool
        foo. -- divided(atDistance: CGFloat, from: CGRectEdge) -> (slice: CGRect, remainder: CGRect)
        foo. -- encode(to: any Encoder) -> Void
        foo. -- height: CGFloat
        foo. -- insetBy(dx: CGFloat, dy: CGFloat) -> CGRect
        foo. -- integral: CGRect
        foo. -- intersection(r2: CGRect) -> CGRect
        foo. -- intersects(r2: CGRect) -> Bool
        foo. -- isEmpty: Bool
        foo. -- isInfinite: Bool
        foo. -- isNull: Bool
        foo. -- maxX: CGFloat
        foo. -- maxY: CGFloat
        foo. -- midX: CGFloat
        foo. -- midY: CGFloat
        foo. -- minX: CGFloat
        foo. -- minY: CGFloat
        foo. -- offsetBy(dx: CGFloat, dy: CGFloat) -> CGRect
        foo. -- origin: CGPoint
        foo. -- self: CGRect
        foo. -- size: CGSize
        foo. -- standardized: CGRect
        foo. -- union(r2: CGRect) -> CGRect
        foo. -- width: CGFloat
  3> foo.maxX
$R0: Foundation.CGFloat = 4
  4> foo.maxY
$R1: Foundation.CGFloat = 6
  5>

@vade
Copy link
Copy Markdown
Collaborator Author

vade commented Sep 26, 2024

Oh thats unexpected. Thanks for the info @jchen9 - I'll def leverage CG* for the types. Thats great news.

@furby-tm
Copy link
Copy Markdown
Contributor

furby-tm commented Sep 27, 2024

@jchen9 thanks for letting us know about CGRect being available in Foundation (cross-platform), are you aware the minimum Swift version that was made available in?

I propose, if it's possible, we try to target a minimum swift-tools-version of 5.9 or 5.10 (currently we are on 5.6) moving forward, as that's also the minimum for Swift/C++ interop, and I think the consensus is that we'd eventually like to deprecate the objective-c layer and make this Swift package available to Linux users as well.

We should also consider duplicating the package manifest code in a Package@swift-5.6.swift if backwards compatibility is important.

@jchen9
Copy link
Copy Markdown
Contributor

jchen9 commented Sep 27, 2024

I can't find prebuilt Swift 5.9 Linux binary from Swift.org. From the history of https://github.com/swiftlang/swift-corelibs-foundation/blob/main/Sources/Foundation/NSGeometry.swift these structs might have been in the Foundation for a while.

@vade
Copy link
Copy Markdown
Collaborator Author

vade commented Mar 28, 2025

Hi there!

If we chose to go w/ Swift 5.9 I presume that also means we are safe to assume Foundation on all Swift supported platforms should have CGRect - therefore its ok to refactor this PR and adopt CGRect to be exposed from the API?

Give a thumbs up and I will try to find some time to work on it and close this out.

Thanks ya'll

@jminor
Copy link
Copy Markdown
Member

jminor commented Mar 28, 2025

Yes, that sounds good to me. See also this PR: #61

vade added 3 commits April 10, 2025 13:05
Signed-off-by: vade <vade@vade.info>
Signed-off-by: vade <vade@vade.info>
…mply use CGRect structs as appropriate.

Signed-off-by: vade <vade@vade.info>
@vade vade force-pushed the feature/available_image_bounds branch from 37b2cbb to 66e3d65 Compare April 10, 2025 17:05
Signed-off-by: vade <vade@vade.info>
@vade vade force-pushed the feature/available_image_bounds branch from 088fa7c to 090246c Compare April 10, 2025 17:37
@vade
Copy link
Copy Markdown
Collaborator Author

vade commented Apr 10, 2025

@jminor apologies for the delay - i pushed the changes, including a small test - i added a clip reference test OTIO sourced from the main OTIO testing resources.

@vade vade changed the title Feature/available image bounds (Draft) Feature/available image bounds Apr 10, 2025
Comment thread Sources/objc/opentimelineio.mm Outdated
Comment thread Sources/swift/MediaReference.swift Outdated
vade added 3 commits April 10, 2025 16:21
…t explaining usage.

Signed-off-by: vade <vade@vade.info>
Signed-off-by: vade <vade@vade.info>
Signed-off-by: vade <vade@vade.info>
@vade
Copy link
Copy Markdown
Collaborator Author

vade commented Apr 10, 2025

Thanks for the prompt review @jminor I cleaned up based off of your suggestions. Much obliged!

@vade vade marked this pull request as ready for review April 10, 2025 20:24
@jminor jminor merged commit b0856e8 into OpenTimelineIO:main Apr 21, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants