Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow CGFloat values to be serialised. #1309

Merged

Conversation

zachsimone
Copy link
Contributor

@zachsimone zachsimone commented Oct 3, 2023

Description & motivation

As the title says, it is handy to be able to map a CGFloat value to a Double for the purposes of encoding it in the Serializer types.

This came about when the VideoSize type went away. It was replaced by a CGSize which returns CGFloat types for the width and height properties. In the makeMetaData function on an RTMPStream, these properties are assigned as values to keys in the dictionary:

metadata["width"] = videoSettings.videoSize.width
metadata["height"] = videoSettings.videoSize.height

The only problem is that they fail to serialise when going through the func serialize(_ value: Any?) -> Self function in AMF0Serializer, as it doesn't handle CGFloat data types.

Please let me know if you foresee any issues with the proposed solution. It should be additive only. In the interest of consistency, I've made the change in both AMF0Serializer and AMF3Serializer. If there's a reason it should be in one and not the other, I'm happy to update.

Alternatives considered

I also considered fixing the problem at the point we add the properties to the dictionary, e.g:

metadata["width"] = Double(videoSettings.videoSize.width)
metadata["height"] = Double(videoSettings.videoSize.height)

But ultimately decided making the serialiser code more robust could help prevent against similar issues in the future, instead of fixing just the specific situation with video resolution here.

If you'd prefer the change above where the values are cast to Double, I'm happy to make that change instead. I understand if there's a reluctance to use a CGFloat type in this library as it's not in the Swift language itself. However, I note CGFloat is used in other places, and that the file I've edited does import Foundation so I figure it should be okay.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

@shogo4405 shogo4405 merged commit dd2622d into shogo4405:main Oct 3, 2023
@shogo4405 shogo4405 added this to the 1.6.1 milestone Oct 3, 2023
@shogo4405
Copy link
Owner

Just as you said.

@zachsimone zachsimone deleted the bugfix/handle-CGFloat-metadata branch October 3, 2023 20:03
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.

2 participants