Video Module: Initial Release #8858

karimMourra · 2022-08-17T19:44:18Z

Type of change

Feature

Description of change

The objective of this PR is to implement the Video Module proposed in #6271 and detailed in the official proposal doc.

contact email of the adapter’s maintainer
karim mourra - karim@jwplayer.com

Documentation PR: prebid/prebid.github.io#4100

Other information

fulfills proposal
Addresses:
#6271
#5696

lgtm-com · 2022-08-17T22:44:18Z

This pull request introduces 1 alert when merging eacbf3a into 059b7c5 - view on LGTM.com

new alerts:

1 for Inclusion of functionality from an untrusted source

lgtm-com · 2022-08-23T21:57:40Z

This pull request introduces 1 alert when merging 128b718 into 8d88902 - view on LGTM.com

new alerts:

1 for Inclusion of functionality from an untrusted source

karimMourra · 2022-08-24T15:16:09Z

cc @FranciscoVergaraC

karimMourra · 2022-08-24T16:16:31Z

@dgirardi ready for round 2 of review

dgirardi · 2022-09-01T16:47:03Z

integrationExamples/videoModule/jwplayer/bidMarkedAsUsed.html

+
+        pbjs.addAdUnits(adUnits);
+
+        pbjs.onEvent('video_setupComplete', (e) => {


minor point - videoSetupComplete looks better to me compared to video_setupComplete.

@dgirardi this has been addressed

dgirardi · 2022-09-01T16:48:02Z

modules/.submodules.json

    ]
+  },
+  "libraries": {


This is unnecessary (and ignored) since #8599

@dgirardi this has been addressed

dgirardi · 2022-09-01T16:57:48Z

modules/jwplayerVideoProvider.js

+
+    switch (type) {
+      case SETUP_COMPLETE:
+        setupCompleteCallback = callback;


does this mean that you never expect multiple listeners for the same event? Is that enforced somewhere?

@dgirardi this has been addressed

dgirardi · 2022-09-01T16:59:43Z

modules/jwplayerVideoProvider.js

+          Object.assign(payload, {
+            sourceError: e.error,
+            errorCode: e.code,
+            errorMessage: e.message


There's some duplication here; all handlers add stuff to the payload and invoke callback with it, which could be extracted out. Error translation is also duplicated across some of them.

@dgirardi this has been addressed

dgirardi · 2022-09-01T17:06:04Z

modules/videoModule/index.js

+    }
+
+    let ortb2 = { ortb2: mergeDeep({}, pbGlobal.getConfig('ortb2'), { site: oRtbParams.content }) };
+    pbGlobal.setConfig(ortb2);


Since v7 this is not the correct way to update first party data. But beyond that, I'm not sure fpd should be updated in this way here. I don't know if I understand the flow well enough, but it looks to me that with this:

there can be max 1 player on the page (that we use to set site.content)

if this module is active, all video bids are assumed to go through this (because we update
mediaTypes.video for all of them).

if 1 makes sense, this should hook into startAuction (instead of requestBids) and modify the ortb2Fragments argument (there's some documentation on this argument in the context of RTD modules; this is the hook).

For 2 I think it'd be better to have a way to say "this bid is for jwplayer" (or videoCore, not sure about the name), to allow other types of video bids to stay unmolested.

@dgirardi great observation. One of the key problems we aim to solve with the Video Module, is to automatically populate contextual information in the bid by getting the data from the source of truth, which is the video player.

Each video provider gathers ortb data about the video player and its content. At the requestBids time, the information is extracted and added to adUnit.mediaTypes.video and config.ortb2.site. Note that this only happens when the ad unit's mediaType is video and the adUnit is configured to use the Video Module (indicated by populating adUnit.video).
Since we only modify adUnit.mediaTypes.video when the ad unit references a specific video player, it seems like point 2 is addressed.
However, point 1 is a problem. We want to support multiple players on the page, where an ad unit can be associated to a specific player. I think this makes sense since an ad unit represents a placement, and the placement is in a specific player. We want the bid request to include the contextual information in $.site.content. Would this work by populating ortb2Fragments at startAuction ?

I stand corrected on point 2; I did not realize that adUnit.video is required.

For point 1, unfortunately I don't think there's a good way currently to manage multiple distinct site.content within the same auction. I also wonder - if a separate, unrelated slot (for example a banner) is somewhere on the page, is it a problem if it gets site.content from one of the players on the page?

Since v7, the guidance for adapters is to take for first party data from bidderRequest.ortb2, which is the same for all impressions. The same ortb2 object is also repeated in individual bidRequest objects; you could hook into that logic so that the video-specific site.content is only added to video bid requests, and that would allow you different data for different players.

However, that requires a refactoring of most adapters - because currently they all assume that there is only one ortb2 shared between all impressions.

An alternative would be to propose this data somewhere else under imp (maybe imp.ext.content?); similar to what is being discussed for source.tid (another field that turns out should have been defined under imp).

Another alternative still could be requiring these ad units to have their own auction - hook into ad unit validation to enforce that you cannot mix video players, or mix these new video slots with other media types. This way, all impressions for video auctions can use the same site.content, that you can set from startAuction. This is the easiest to implement but I'm not sure how much of a nuisance it would be for pubs. Maybe it's a good starting point?

I think the fundamental issue is that the oRTB spec allows a bid request to have multiple impressions, but a bid request can only have 1 content. The content block is meant to describe the page (or app), not the placement per say. The description of the page is meant to include metadata from the media on the page.

In the case where a banner imp is included, I don't think it's a problem for the bid request to include the site.content metadata specific to the video on the page, since it is describing the page on which the banner will be displayed. A banner doesn't have any content, but it would still be valuable for the DSPs to know what video content is on the page alongside the banner. They would potentially want to place a banner ad related to the video content.

I don't like the imp.ext.content because the idea is for DSPs to consume the contextual information, and since exts are not standard, they would not read from there.

I think the main problem is in the case where there are multiple players on a page, and the auction includes imps for different players.

Ortb 2.5 defines site.content as "Details about the Content (Section 3.2.16) within the site." - Perhaps we should add a flag on the video config to allow the publisher to indicate which of the video players is the main one. The use case I'm thinking of is a site with an article and a large player at the top. At the bottom of the page there might be smaller players with recommended content.
If an auction is made with multiple imps, the main player's content will be used. Otherwise the content from the specific player associated to the ad unit will be used.

dgirardi · 2022-09-01T17:21:43Z

modules/videoModule/index.js

+  }
+
+  function auctionEnd(auctionResult) {
+    auctionResult.adUnits.forEach(adUnit => {


I think we discussed this already, but this looks unlike Prebid to me. Why not allow the publisher to choose when and which bid to render?

The listener for auctionEnd is only added if the bid request does not have a bidsBackHandler

function beforeBidsRequested(nextFn, bidRequest) { const adUnits = bidRequest.adUnits || pbGlobal.adUnits || []; adUnits.forEach(adUnit => { enrichAdUnit(adUnit); }); const bidsBackHandler = bidRequest.bidsBackHandler; if (!bidsBackHandler || typeof bidsBackHandler !== 'function') { pbEvents.on(CONSTANTS.EVENTS.AUCTION_END, auctionEnd); } return nextFn.call(this, bidRequest); }

dgirardi · 2022-09-01T17:29:56Z

modules/spotxBidAdapter.js

@@ -149,7 +149,7 @@ export const spec = {
        }
      }

-      const mimes = getBidIdParameter('mimes', bid.params) || ['application/javascript', 'video/mp4', 'video/webm'];
+      const mimes = deepAccess(bid, 'mediaTypes.video.mimes') || getBidIdParameter('mimes', bid.params) || ['application/javascript', 'video/mp4', 'video/webm'];


How are you choosing which adapters to update - and do we need to run this through them? Here for example I think the changes might be breaking; I can imagine a publisher that is using spotx but not this new video module, with video adUnits that would work before, but not after, this change.

This change is not specific to the video module. We have been pushing SSPs to read standard oRTB video params from the mediaTypes.video instead of from bid.params. This way the publisher can define the params in one place, instead of copying them for every SSP.
https://docs.prebid.org/dev-docs/adunit-reference.html#adunitmediatypesvideo

Agreed, mediaTypes.video is the "right" way, but what I'm not sure about is the precedence; I think if this was picking from bid.params falling back to mediaTypes.video it would be OK; but the other way around can potentially break some publishers.

The former can only add to the video request, and would keep all existing fields unchanged. The latter can potentially change them if you set up the adUnit in some (admittedly weird) ways.

I see your point; I agree and will address

@dgirardi this has been addressed

dgirardi

Minor points below, but LGTM.

dgirardi · 2022-10-03T15:44:26Z

modules/videoModule/index.js

+
+  return { init, renderBid, getOrtbVideo, getOrtbContent };
+
+  function beforeBidsRequested(nextFn, bidRequest) {


bidRequest is somewhat confusing IMO, auctionRequest or just request would be better because everywhere else in the codebase a bid request refers to a single bidder-adUnit combination.

dgirardi · 2022-10-03T15:46:47Z

modules/videoModule/index.js

+
+    const bidsBackHandler = bidRequest.bidsBackHandler;
+    if (!bidsBackHandler || typeof bidsBackHandler !== 'function') {
+      pbEvents.on(CONSTANTS.EVENTS.AUCTION_END, auctionEnd);


I don't see where this listener is detached - If I run two auctions - the first without bidsBackHandler, the second with it, I think this would still trigger on the second.

@dgirardi thanks! I just addressed your feedback

patmmccann · 2022-10-13T18:29:58Z

copying some comments from @karimMourra into the thread: here's an explanation for how the Video Module determines if an impression came from a bid

We start tracking bids when the BID_ADJUSTMENT events fires on line https://github.com/jwplayer/Prebid.js/blob/440fdde2ab4bc5fb8265140bc3db506c15b1c17e/modules/videoModule/index.js#L60

We use this event because it fires before the ad is cached.

Tracking a bid as seen in this function https://github.com/jwplayer/Prebid.js/blob/440fdde2ab4bc5fb8265140bc3db506c15b1c17e/modules/videoModule/videoImpressionVerifier.js#L138 consists of generating a UUID and storing metadata from the bid (adId, adUnitCode, requestId, auctionId)

The UUID is then used to modify the ad tag being cached:
https://github.com/jwplayer/Prebid.js/blob/440fdde2ab4bc5fb8265140bc3db506c15b1c17e/modules/videoModule/videoImpressionVerifier.js#L109 and https://github.com/jwplayer/Prebid.js/blob/440fdde2ab4bc5fb8265140bc3db506c15b1c17e/modules/videoModule/videoImpressionVerifier.js#L111

When a video Impression (or Error) is captured, triggerVideoBidEvent is called, at line https://github.com/jwplayer/Prebid.js/blob/440fdde2ab4bc5fb8265140bc3db506c15b1c17e/modules/videoModule/index.js#L206 which results in attempting to determine if the ad event is related to a bid. This check is done by determining if the ids surfaced in the ad event payload match the UUID generated earlier in https://github.com/jwplayer/Prebid.js/blob/440fdde2ab4bc5fb8265140bc3db506c15b1c17e/modules/videoModule/videoImpressionVerifier.js#L145 . If there is a match, then the bid metadata that was stored with the UUID (adId, adUnitCode, requestId, auctionId) is obtained and is then used to mark the right bid as won.

The adId that comes from the ad event payload, is used to match to the UUID, as well as the array of wrapper ids, and the actual ad tag url since we also append the UUID as a query param. If the id from the event payload matches one of our UUIDs, we return the bid metadata that was stored.

this block ensures we have the right bid

const { adUnitCode, requestId, auctionId } = bidIdentifiers; const bidAdId = bidIdentifiers.adId; const { bids } = pbGlobal.getBidResponsesForAdUnitCode(adUnitCode); return find(bids, bid => bid.adId === bidAdId && bid.requestId === requestId && bid.auctionId === auctionId);
where bidIdentifiers is the bid metadata

karimMourra · 2022-10-21T18:34:42Z

Hi @bwschmidt thanks for the review! I added docs to the modules

patmmccann · 2022-10-21T18:46:37Z

Thanks @bwschmidt !!

* registers as module * adds vendor consts * registers jwplayer submodule * renames render ad * checks config autostart * moves private functions to a util obj * introduces ad optimization * tests state.js * introduces submodule tests * populates utils tests * tests callback storage * tests ad and time states * introduces provider tests: * tests init * tests get ortb * tests event listeners * supports removing all event handlers when explicit * splits clear all event listeners from early return * removes hook * shares directory * prevents registering a same provider twice * adds pb video * adds video module to submodules list * adds integration example * implements ad unit enrichment * moves submodule to parent dir * removes comment * implements event listener * adds unit tests * tests updating video config * tests events surfacing * nests the event trigger tests * restores factory * adds adServer * shares base parent module logic * register gam submodule * tests submodule builder * instantiates ad server * tests parent module * updates core video tests * adds ad server tests * obtains vast ad to render * passes options * tests ad tag obtention * tests settings tags without ad server * adds vast xml builder * tests node makers * implments vast xml build fn * renames wrapper builder * adds vast xml editor * modifies vast xml * moves logic to helper * appends doc element * copies node when required multiple times * improves copy check * adds tests for error node * adds tests cases for different config * pulls vast wrapper builders from xml editor * adds tracking tests * tests arguments * removes obsolete path * polls for duration when absent * sets up video module * prevents ad load if bidding is used * triggers event before loading ad * adds inline docs to shared modules * adds inline docs * documents providers * adds inline docs for ortb * adds missing ortb params * obtains language code * makes placement more precise * adds notes * updates todo * early exit if not video media type * loads ad server config * filters for highest bids before rendering * emits event even on loss * registers video auction events * updates events * includes ad unit code in loss * uses config fallback * tracks bids sent for rendering * adds video impression verification * shares code * triggers bid video events * matches ad event to bid * adds inline documentation * allows URL import * updates vast xml editor tests * tests ad event processsing * includes ad wrapper ids * updates ad tag injection tests * tests impression tracking * updates for new find * removes unused var * uses find polyfill * updates event import * exposes addEvents * checks for bid identifiers bfore accessing properties * checks for bid identifiers bfore accessing properties * exposes video bid events * converts ortb params to insternal fields * reads from video * removes typo * sets ortb video to media type video * sets ortb2 obj * gets impression url from fn * tmg uses mediatype video * configures provider to properly render ads * sets up player in demo * triggers load abort * uses appnexys * uses getGlobal * protects against missing params * breaks out shared and adserver from videoModule * updates import * resolves lgtm errors * uses submodule pattern * uses uuid generator util * removes adServer module * moves shared to video folder * prefix events with video_ * adds auction end listener when no bids back handler * fixes tests * refactors render * exposes rendering * uses mergeDeep * shares constants * populates ortb content.data * register events as prefixed * adds test bidders * moves videoModule to library * Added draft for videojs integration example * Moved jwplayer integration example, added draft for videojs example * Created integration example for a mocked videojs adapter * Fixed call order for integration examples, moved imports up * Finished init for videojs provider * Setup videojs init unit tests * Formatting to pass linter * Removed comments/redundant tests, clarified some sections of code * Added semicolon * Videojs ortb params, added ortb constants * Fixed integration examples, ortb adjustments * Cleaned up code added comments * Renamed test file * Removed videojs mock in favor of videojs * Changed video source, finished tests for ortb * Cleaned up spacing added some comments * Added ad position to ortb * Cleaned up playback methods code * Omitted skip variable per ortb spec * Added unit tests for ima integration * Added util function, and corresponding tests * Added code for placement * Linter pass through * Added videojs-ima to integration example * Removed duplicate tests * Removed non bidding module tests * Added clarifying comments * Updated name mapping for videojs events * Added callback factory, and presetup listeners * Filled in initialization events * Importing event constants * Fixing setup failed event handler * Fixed init behavior to work with setupCompleteCallback * Added pre setup callback tests * Setup presetupListeners/moved away from callbackStorage * Added missing case for setup failure * Basic event handling * adds ortb constants * removes trailing file * uses hook.js * marks winning bid as used * gets adWrapper regardless of property name * rename vars * fixes init and loadAdTag * renames videojs var * supports media events * addresses size events * registers ad events * implements get ad info * generates ad pauload * includes wrapper ids * supports ad config * populates ad config * adds event listener at every manager change * handles setupfailure * renames video js example * updates time * supports loadstart * supports multiple setup callbacks * removes global ima for test * removes obsolete IMA checks * requests ad unit * uses vendor config to setup * listens for playlist events * restors src tag * resolves build fail * splits library from module * allows obtaining ortb obj * uses safe getConfig * adds metadata demo * improves metadata util * triggers playlist event at first load * adds bid request scheduling demo * obtains bid adId * sets proper context * combines all possible adServer configs * configures ad tag * adds demos * adServerConfig getter is nullable * adds demos * triggers setup fail when ima is missing * checks for player existence * updates tests * adds tests * tests media parsers * returns empty object when no config gound * adds integrity * removes library map * prioritizes bidder params * uses camel casing for event names * support multiple setup successful callbacks * supports multiple setup fail ballbacks * moves early return to core * registers one event at a time * uses event handler builder * uses name conversion util * refactors event handling * refactors ima listeners * simplifies event logic * checks for existence * offevent singular * adds fn desc * splits ortb video from content * checks for player existence * enrichment rules * only sets main content div when enabled * sets ortb2 in the bidRequest * tests re ortb * refactor tests * resolves instability * removes auction end event listener * rebuilds dependencies * sets ortb site.content * renames auctionRequest * renames optimizeAds * adds module docs * adds event doc Co-authored-by: karimJWP <karimJWP@github.com> Co-authored-by: AnirudhRahul <anirudhrahul@anjurahul.com>

karimMourra added the in progress label Aug 17, 2022

karimMourra requested a review from patmmccann August 17, 2022 19:44

karimMourra mentioned this pull request Aug 17, 2022

Marking used bids for video #5696

Closed

karimMourra force-pushed the feat/AD-1416 branch 2 times, most recently from d96fa26 to a0ad24f Compare August 17, 2022 20:03

patmmccann linked an issue Aug 17, 2022 that may be closed by this pull request

Marking used bids for video #5696

Closed

karimMourra removed the in progress label Aug 24, 2022

karimMourra requested a review from dgirardi August 24, 2022 16:15

dgirardi reviewed Sep 1, 2022

View reviewed changes

karimMourra force-pushed the feat/AD-1416 branch from fbdae1b to 06b4f83 Compare September 9, 2022 23:54

dgirardi mentioned this pull request Sep 29, 2022

How to make analytics module able to track instream bidwon event #9055

Closed

karimMourra force-pushed the feat/AD-1416 branch from 440fdde to 149be56 Compare September 30, 2022 21:27

dgirardi approved these changes Oct 3, 2022

View reviewed changes

karimMourra force-pushed the feat/AD-1416 branch from 6cacb99 to ceb721b Compare October 3, 2022 18:37

karimMourra added the LGTM label Oct 3, 2022

ChrisHuie added feature needs docs and removed LGTM needs docs labels Oct 6, 2022

karimMourra requested review from ChrisHuie and jsnellbaker October 6, 2022 15:29

ChrisHuie assigned robertrmartinez Oct 8, 2022

ChrisHuie removed their request for review October 11, 2022 15:40

ChrisHuie added the needs docs label Oct 11, 2022

karimMourra added 23 commits October 21, 2022 11:47

uses event handler builder

cb257a3

uses name conversion util

abbd00c

refactors event handling

2b20e62

refactors ima listeners

3389600

simplifies event logic

630ea7d

checks for existence

7dcfaf9

offevent singular

c99c9ca

adds fn desc

86834d5

splits ortb video from content

5653a25

checks for player existence

482eb6c

enrichment rules

86f7b3e

only sets main content div when enabled

d684804

sets ortb2 in the bidRequest

f760063

tests re ortb

ce7aa3d

refactor tests

eaef25f

resolves instability

04280c2

removes auction end event listener

bd26b76

rebuilds dependencies

2e4e8a1

sets ortb site.content

cc87757

renames auctionRequest

5df9f4b

renames optimizeAds

4a5ddef

adds module docs

6d4aae5

adds event doc

896a11a

karimMourra force-pushed the feat/AD-1416 branch from c41f286 to 896a11a Compare October 21, 2022 18:33

patmmccann merged commit 0c69238 into prebid:master Oct 21, 2022

bretg unassigned robertrmartinez Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Video Module: Initial Release #8858

Video Module: Initial Release #8858

karimMourra commented Aug 17, 2022 •

edited

Loading

lgtm-com bot commented Aug 17, 2022

lgtm-com bot commented Aug 23, 2022

karimMourra commented Aug 24, 2022

karimMourra commented Aug 24, 2022

dgirardi Sep 1, 2022

karimMourra Sep 12, 2022

dgirardi Sep 1, 2022

karimMourra Sep 12, 2022

dgirardi Sep 1, 2022

karimMourra Sep 12, 2022

dgirardi Sep 1, 2022

karimMourra Sep 12, 2022

dgirardi Sep 1, 2022

karimMourra Sep 6, 2022

dgirardi Sep 6, 2022

karimMourra Sep 7, 2022

karimMourra Sep 7, 2022

dgirardi Sep 1, 2022

karimMourra Sep 7, 2022

dgirardi Sep 1, 2022

karimMourra Sep 7, 2022

dgirardi Sep 7, 2022

karimMourra Sep 7, 2022

karimMourra Sep 12, 2022

dgirardi left a comment

dgirardi Oct 3, 2022

dgirardi Oct 3, 2022 •

edited

Loading

karimMourra Oct 3, 2022

patmmccann commented Oct 13, 2022 •

edited

Loading

karimMourra commented Oct 21, 2022

patmmccann commented Oct 21, 2022


		pbjs.addAdUnits(adUnits);

		pbjs.onEvent('video_setupComplete', (e) => {


		return { init, renderBid, getOrtbVideo, getOrtbContent };

		function beforeBidsRequested(nextFn, bidRequest) {

Video Module: Initial Release #8858

Video Module: Initial Release #8858

Conversation

karimMourra commented Aug 17, 2022 • edited Loading

Type of change

Description of change

Other information

lgtm-com bot commented Aug 17, 2022

lgtm-com bot commented Aug 23, 2022

karimMourra commented Aug 24, 2022

karimMourra commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmmccann commented Oct 13, 2022 • edited Loading

karimMourra commented Oct 21, 2022

patmmccann commented Oct 21, 2022

karimMourra commented Aug 17, 2022 •

edited

Loading

dgirardi Oct 3, 2022 •

edited

Loading

patmmccann commented Oct 13, 2022 •

edited

Loading