Fix vmap mid roll ad position reporting #83
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
strangesource
force-pushed
the
fix-vmap-mid-roll-ad-position-reporting
branch
from
f46e691 to
0a229a1
Compare
krocard
approved these changes
Sep 24, 2024
| verify { videoAnalytics.reportAdBreakStarted(any(), any()) } | ||
| player.listeners[PlayerEvent.AdStarted::class]?.forEach { it(TEST_AD_STARTED_EVENT) } | ||
| verify { | ||
| adAnalytics.reportAdStarted(match { it["c3.ad.position"] == AdPosition.PREROLL }) |
There was a problem hiding this comment.
I didn't know this match helper. Nice!
Comment on lines
276
to
290
| ad = object : Ad { | ||
| override val clickThroughUrl: String? | ||
| get() = "clickThroughUrl" | ||
| override val data: AdData? | ||
| get() = null | ||
| override val height: Int | ||
| get() = 100 | ||
| override val id: String? | ||
| get() = null | ||
| override val isLinear: Boolean | ||
| get() = true | ||
| override val mediaFileUrl: String? | ||
| get() = null | ||
| override val width: Int | ||
| get() = 200 |
There was a problem hiding this comment.
Super nit. Why implement this though getters rather than fields?
Suggested change
| ad = object : Ad { | |
| override val clickThroughUrl: String? | |
| get() = "clickThroughUrl" | |
| override val data: AdData? | |
| get() = null | |
| override val height: Int | |
| get() = 100 | |
| override val id: String? | |
| get() = null | |
| override val isLinear: Boolean | |
| get() = true | |
| override val mediaFileUrl: String? | |
| get() = null | |
| override val width: Int | |
| get() = 200 | |
| ad = object : Ad { | |
| override val clickThroughUrl = "clickThroughUrl" | |
| override val data = null | |
| override val height = 100 | |
| override val id = null | |
| override val isLinear = true | |
| override val mediaFileUrl = null | |
| override val width = 200 |
There was a problem hiding this comment.
7ee0bd3 I went for a mock to make it easier to read.
krocard
approved these changes
Sep 24, 2024
|
|
||
| // mid-roll ad break is scheduled at 15 seconds, will play immediately in case of the DASH | ||
| // live stream due to absolute time stamps. | ||
| player.expectEvent<PlayerEvent.AdBreakStarted> { |
There was a problem hiding this comment.
shouldn't callAndExpectEvent be used instead? The AdBreak start could be missed.
There was a problem hiding this comment.
Yes, in theory this could be cary. Unfortunately the test helpers seem to have an issue that cause a deadlock when loading in the callAndExpectEvent. 🙈 I'll keep it as-is. We should have a look at the test helpers at some point.
matamegger
approved these changes
Sep 24, 2024
conviva/src/test/kotlin/com/bitmovin/analytics/conviva/ConvivaAnalyticsIntegrationTest.kt
Outdated
Show resolved
Hide resolved
conviva/src/test/kotlin/com/bitmovin/analytics/conviva/ConvivaAnalyticsIntegrationTest.kt
Outdated
Show resolved
Hide resolved
…AnalyticsIntegrationTest.kt Co-authored-by: Matthias Tamegger <[email protected]>
…AnalyticsIntegrationTest.kt Co-authored-by: Matthias Tamegger <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Ad position reporting for client side ad insertion with VMAP and Google IMA ads integration was reporting wrong ad position for mid- and post-roll ads.
Changes
As the timing information on the ad is not reported correctly for VMAP ads (tracked internally), we take the correct time from the current active ad break. This is in line with the tacking on web.