-
Notifications
You must be signed in to change notification settings - Fork 11
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
SSAI tracking API #78
Conversation
let mergedAdInfo = contentMetadataBuilder | ||
.build() | ||
.merging(adInfo) { $1 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD if we want to carry over the already collected metadata.
On iOS this was needed to fix missing player name and viewer ID errors, despite the documentation stating those would be auto-collected from the videoAnalytics
instance (they are not in practice).
This will take adInfo with priority over content metadata when values exist for both.
@@ -76,9 +76,15 @@ class CISAdAnalyticsTestDouble: NSObject, CISAdAnalyticsProtocol, TestDoubleData | |||
} | |||
|
|||
func reportAdMetric(_ key: String, value: Any) { | |||
let valueToTrack: String | |||
if let size = value as? CGSize { | |||
valueToTrack = "\(size.width)x\(size.height)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a workaround for testing as printing a CGSize
value can have 2 different formats somehow.
spyResult = spyTracker.hasCalledFunction(functionName, withArgs: expectedArgs) | ||
argsAreMatching = spyResult.success | ||
|
||
let messageString = """ | ||
have called <\(functionName)> with args<\(expectedArgs)> \ | ||
have called <\(functionName)> \ | ||
with args<\(expectedArgs.toStringWithStableOrder())> \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small little fix to print expected values ordered
CIS_SSDK_METADATA_VIEWER_ID, | ||
CIS_SSDK_METADATA_PLAYER_NAME, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are listed in the Conviva documentation as "autocollected" from the main video session but during testing they were showing up as error due to missing values in the ad session, therefore we decided to explicitly set these for the ad info.
# Conflicts: # BitmovinConvivaAnalytics/Classes/ConvivaAnalytics.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my limited knowledge of the language, this looks very good. 👍
Just some minor comments/questions from my side.
@@ -343,6 +384,23 @@ public final class ConvivaAnalytics: NSObject { | |||
buildDynamicContentMetadata() | |||
} | |||
|
|||
private func buildAdMetadata() -> [String: Any] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Should have some indication in the name that this is for server-side ads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, renamed in 54d8d54
/// Reports the start of a server-side ad. | ||
/// | ||
/// - Parameter adInfo: Object containing metadata about the server-side ad. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Could use some documentation that there needs to be an active ad break first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, fixed in 2e0c820
@@ -343,6 +384,23 @@ public final class ConvivaAnalytics: NSObject { | |||
buildDynamicContentMetadata() | |||
} | |||
|
|||
private func buildAdMetadata() -> [String: Any] { | |||
let includedTags: Set<String> = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't carry over the asset name yet. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is a mistake, I missed that one. Fixed in 6fbe0f6
Co-authored-by: Lukas Knoch-Girstmair <strangesource@users.noreply.github.com>
@strangesource FYI I just added an SSAI README section and aligned the CHANGELOG entry in 4b92076 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Problem
We don't support tracking SSAI ads via the Conviva integration.
Solution
Introduced
ConvivaAnalytics.ssai
property withSsaiApi
type which has:SsaiApi.isAdBreakActive
for checking if an SSAI ad break is activeSsaiApi.reportAdBreakStarted()
for reporting SSAI ad break started eventsSsaiApi.reportAdBreakFinished()
for reporting SSAI ad break finished eventsSsaiApi.reportAdStarted()
for reporting SSAI ad started eventsSsaiApi.reportAdFinished()
for reporting SSAI ad finished eventsSsaiApi.reportAdSkipped()
for reporting SSAI ad skipped eventsSsaiApi.update(adInfo:)
for updating the ad infoNotes
The example application has no way of testing this built-in.
Checklist
CHANGELOG.md
file