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

Wire up SSAI #73

Merged
merged 8 commits into from
Jul 3, 2024
Merged

Wire up SSAI #73

merged 8 commits into from
Jul 3, 2024

Conversation

strangesource
Copy link
Contributor

@strangesource strangesource commented Jul 2, 2024

Problem

The SSAI API introduced in #72 is not yet used.

Changes

  • Add SSAI API to ConvivaAnalyticsIntegration
  • Expose SSAI namespace on ConvivaAnalyticsIntegration
  • Reset SSAI state on internal session end
  • Report stream error as ad error in case of active SSAI ad
  • Add first instrumentation test

Limitations

On SSAI ad start we currently don't report initial values for resolution, bitrate and framerate. This will be done in a follow up PR.

Related Changes

#69
#72

@strangesource strangesource self-assigned this Jul 2, 2024
Comment on lines 143 to 145
private boolean isAdActive() {
return bitmovinPlayer.isAd();
return bitmovinPlayer.isAd() || ssai.isAdBreakActive();
}
Copy link
Contributor Author

@strangesource strangesource Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implicitly enables tracking of

  • PLAY_HEAD_TIME
  • PLAYER_STATE
    on the ConvivaAdAnalytics as it is already tracked for client side ads.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reporting the play head time, is the current format what the conviva backend expects or do we have to convert the time to a relative timestamp since the ad(break) started? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent question. I would assume no but let me double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there is no documentation to be found about this. Let's leave it as-is for now as it makes the most sense for SSAI ads.

@strangesource strangesource changed the title Wire up ssai Wire up SSAI Jul 2, 2024
@strangesource strangesource requested a review from matamegger July 2, 2024 13:50
@strangesource strangesource marked this pull request as ready for review July 2, 2024 13:51
Copy link
Contributor

@matamegger matamegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! Nice and easy to read solution 👍

Base automatically changed from add-ssai-api to develop July 2, 2024 14:54
Copy link

@rolandkakonyi rolandkakonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment on the SSAI API integration code as it was already merged before but I think we should consider taking the tags produced by contentMetadataBuilder.build() when passing adInfo.

On the iOS integration I had to do that to fix errors showing up due to missing tags, like viewer ID and player name. These are documented as "Autocollected" but in practice that didn't happen on iOS.

The other option could be that on the iOS integration we only "patch" the "Autocollected" values.

What do you think?

@strangesource
Copy link
Contributor Author

strangesource commented Jul 3, 2024

I can't comment on the SSAI API integration code as it was already merged before but I think we should consider taking the tags produced by contentMetadataBuilder.build() when passing adInfo.

On the iOS integration I had to do that to fix errors showing up due to missing tags, like viewer ID and player name. These are documented as "Autocollected" but in practice that didn't happen on iOS.

The other option could be that on the iOS integration we only "patch" the "Autocollected" values.

What do you think?

I am a bit reluctant to just pass everything, as metadata might be explicitly set for the main content and not relevant for ads. The "patch auto collected" seems like a safe approach here.

Edit: As discussed, on Android we don't seem to have these issues.

@rolandkakonyi
Copy link

I can't comment on the SSAI API integration code as it was already merged before but I think we should consider taking the tags produced by contentMetadataBuilder.build() when passing adInfo.
On the iOS integration I had to do that to fix errors showing up due to missing tags, like viewer ID and player name. These are documented as "Autocollected" but in practice that didn't happen on iOS.
The other option could be that on the iOS integration we only "patch" the "Autocollected" values.
What do you think?

I am a bit reluctant to just pass everything, as metadata might be explicitly set for the main content and not relevant for ads. The "patch auto collected" seems like a safe approach here.

Edit: As discussed, on Android we don't seem to have these issues.

Agree, after having a sleep on it came the "patch" idea up. I will continue with that approach.

@rolandkakonyi rolandkakonyi self-requested a review July 3, 2024 08:44
@strangesource strangesource merged commit 2ce9997 into develop Jul 3, 2024
3 checks passed
@strangesource strangesource deleted the wire-up-ssai branch July 3, 2024 08:46
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.

3 participants