googleads / videojs-ima

IMA SDK Plugin for Video.js
Apache License 2.0
450 stars 284 forks source link

omidMode seems to be inverted #1056

Closed cristianpacu-ml closed 2 years ago

cristianpacu-ml commented 2 years ago

it looks like the implementation of omidMode in src/client-side/sdk-impl.js might be inverted. Right now it is implemented like this:

if (this.controller.getSettings().omidMode) {
  adsRequest.omidAccessModeRules = {};
  const omidValues = this.controller.getSettings().omidMode;

  if (omidValues.FULL) {
    adsRequest.omidAccessModeRules[google.ima.OmidAccessMode.FULL] = omidValues.FULL;
  }
  if (omidValues.DOMAIN) {
    adsRequest.omidAccessModeRules[google.ima.OmidAccessMode.DOMAIN] = omidValues.DOMAIN;
  }
  if (omidValues.LIMITED) {
    adsRequest.omidAccessModeRules[google.ima.OmidAccessMode.LIMITED] = omidValues.LIMITED;
  }
}

This means that when the dictionary is created, we assign the vendor to de access mode:

{
  <Access Mode>: <Vendor>
}

According to the documentation, the implementation should be the other way around:

{
  <Vendor>: <Access Mode>
}

I suggest the following change:

if (this.controller.getSettings().omidMode) {
  adsRequest.omidAccessModeRules = {};
  const omidValues = this.controller.getSettings().omidMode;

  if (omidValues.FULL) {
    omidValues.FULL.forEach((vendor) => {
      adsRequest.omidAccessModeRules[vendor] = google.ima.OmidAccessMode.FULL
    })
  }
  if (omidValues.DOMAIN) {
    omidValues.DOMAIN.forEach((vendor) => {
      adsRequest.omidAccessModeRules[vendor] = google.ima.OmidAccessMode.DOMAIN
    })
  }
  if (omidValues.LIMITED) {
    omidValues.LIMITED.forEach((vendor) => {
      adsRequest.omidAccessModeRules[vendor] = google.ima.OmidAccessMode.LIMITED
    })
  }
}

omidValues would also be a dictionary where the key is still the access mode and the value is an array of vendors to be assigned to that access mode. E.g:

const options = {
  ..., // Some videojs options
  omidMode: {
    "FULL": [googleSDK.ima.OmidVerificationVendor.OTHER],
    "DOMAIN": [googleSDK.ima.OmidVerificationVendor.GOOGLE]
  }
}
this.videoJSPlayer.ima(options)
Kiro705 commented 2 years ago

Hi @cristianpacu-ml ,

Thanks for catching this, it looks like it was not updated with the OMID access mode API change on 12/16/2021.

I will work on making this change in an upcoming release.

Jackson IMA SDK team

Kiro705 commented 2 years ago

Code updated in https://github.com/googleads/videojs-ima/commit/bb3a8e0eb0f65f7c5d3958f514f36dffda54b4b3

Will go live in the upcoming NPM release.

Kiro705 commented 2 years ago

This fix is available in v2.1.0.