Video Module & JwPlayer video submodule : Initial Releases

[karimMourra]

Type of change

• Feature

Description of change

This PR is a Work in Progress and this branch is meant to be a feature branch. As components are implemented, PRs will be opened against this branch, allowing the community to review changes incrementally.
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

Other information

fulfills proposal
#6271

[patmmccann]

@jimdigriz check this and https://github.com/jwplayer/Prebid.js/pull/5/files as an example of video player submodules to a video module. Curious for your feedback.

[jimdigriz]

@karimMourra sent me an older revision of the proposal that I commented on. This time around I now understand more about what it is trying to solve and it looks really useful. I am not a publisher so can only comment wearing an analytics/RTD hat.

I know the proposal states the scope as being 'video' here but as they are so close in implementation I think a statement about audio ads should be made, even if that statement is simply 'out of scope'. A description of what future work might look like and how it could build on the video module to support audio would be useful.

I do like the event hooks as they extend the existing events in an expected manner and are straight forward to use. They provide a technical solution for analytics purposes so it is no longer required to use an 'ad server' module to create a VAST wrapper to crowbar in a VPAID shim.

I do not really understand why getOrtbParams() exists. Currently it resembles a 'capabilities' list and where it does not it mostly reflects the configuration passed to it (eg. h, w, ...) all of which should probably should be in the SETUP_COMPLETE event and then handled by VideoCore to set up the oRTB parameters there. It should also be described how the publisher could post-hook to fix up situations where a video player claims VAST 3 or later but does not support Ad Pods (eg. Teads, or they at least used to).

For all the VideoCore methods detailed in the proposal, instead of divId I would expect AdSlot which would be more consistent with other APIs found in Prebid.js.

Now for a bit of a larger thinking point, what would be interesting is if video sub-modules (actually all modules) were forced to make HTTP requests through Prebid.js:

1. publisher gains full control and enforcement (including cookie/tracking) of what is allowed
2. there is the option to provide hooks where another module can (optionally) rewrite the request and response.

Slightly off topic, now might be a good time to start looking to enforce this policy (starting in VideoCore) as there do not look to be many direct users (non-ajax.js):

alex@aineko:~/src/Prebid.js$ $ git grep -l -e sendBeacon -e 'new Image' -e 'XMLHttp' -e 'create.*img' -e 'fetch(' modules/
modules/adoceanBidAdapter.js
modules/adomikAnalyticsAdapter.js
modules/browsiRtdProvider.js
modules/craftBidAdapter.js
modules/mantisBidAdapter.js
modules/sharethroughAnalyticsAdapter.js
modules/smaatoBidAdapter.js
modules/sortableBidAdapter.js
modules/yuktamediaAnalyticsAdapter.js

Reworking ajax.js so that there is additionally a fetch()-esque API wrapper option would handle their needs of a non-blocking background/<img ...> tracking. No need to add this functionality right now but forcing modules to push the request through Prebid.js means someone can add this functionality at a later date when they need it.

Back on topic, the ability to add a hook to rewrite the request/response (breakpointing on redirects) would make for a powerful tool.

One use case I can think of is if a publisher is trying to insert a verification partner. Ideally everything would support OMSDK/OMID (hah!) so the publisher could amend the VAST response to include further partners (if they are unable to make this change on their ad server) but when the publisher does not they can call out instead to a VAST/VPAID wrapper; of course if the video player says no to VPAID they can fallback further to event tracking pixels.

Another use case is to extract data from the <Extensions> block.

Though proposed, I do not actually see the need for a VAST parser, exposing the chain of VAST responses as XML documents makes for querySelector() being a really familiar way to interact with them. Maybe later Prebid.js may wish to provide a number of helper tools (I am thinking more utils.js/querySelector() orientated rather than vast.js) once the dust settles and code/function duplication becomes apparent.

[karimMourra]

Hi @jimdigriz thanks for the feedback.
getOrtbParams() will be called by the the module that bridges Prebid with Video-Core - let's call it Prebid-video. Prebid-Video will listen for Prebid hooks, say before: bids requested, call getOrtbParams() and write the result to the ad units, for the bid adapters to consume. This way the video params are provided by the Video Players, which are the subject matter experts, instead of the publisher's best guess. The reason all those params aren't passed with the SETUP_COMPLETE event is because they may change during the player's lice cycle, and in the case where a player is configured to play midrolls and postrolls, it's best to obtain the up to date oRTB params right before the auction, which may happen right before the ad's time cue.

Audio Ads is a good idea, I remember it from our previous conversation, but I would like more feedback from the video player community before proceeding; the way jwplayer handles audio is detecting the item's media type and/or config options; i would rather see how the rest of the community handles audio before proposing a solution.

AdSlot vs divId is an interesting proposal. I think that should work.

Re: http requests, are you talking about the requests to the ad server to get the ad creative? and the requests to the content servers to get the content ? Those are handled internally by the players; I don't think it would be feasible for the video players to request via Prebid

[jimdigriz]

Re: http requests, are you talking about the requests to the ad server to get the ad creative? and the requests to the content servers to get the content ? Those are handled internally by the players; I don't think it would be feasible for the video players to request via Prebid

Hoping for as much as the requests as practical but you are right, outsourcing the actual media requests would be awkward.

Maybe an easier approach which involves no changes is to make law that all video players must support URL.createObjectURL() URLs so a publisher could do all the legwork involved in fetching the VAST tag (and making amendments if they wish) and then just pass in a resulting object URL using your existing proposed interface. Those not wishing to do that, can simply pass through the URL from the auction unchanged.

Our video module actually does this and wraps up the VAST tag with our changes and presents the resulting object URL: #6309

[robertrmartinez]

Is this still a draft?

Assigning to @jsnellbaker for now so we do not lose track.

@karimMourra Please let us know when this is ready

[karimMourra]

Hi @robertrmartinez and @jsnellbaker: the modules that have been implemented are fully unit tested and can be merged. They are not demoable or usable without the remaining modules. If you prefer, we can continue adding the remaining modules to this PR as they get implemented, or we can merge this PR and open separate PRs for the remaining modules as we go. The latter would result in smaller PRs that are easier to review instead of one large PR. I am open to either option.

[jsnellbaker]

Hi @karimMourra

I personally would like to see more modules added here to that we can see a working demo. If also it's possible, would you be able to add comments/descriptions to the various files to denote what their purpose and what they're meant to support in terms of the workflow? If needed, please refer to the top of the modules/userId/index.js and its related files for what I mean.

Additionally, for the various 'constants' files, would it be possible to revise them to use a json structure; similar to what's used in the src/constants.json for core prebid.js? It would simplify the import statements to perhaps a one/few parent variables, instead of the dozens of individual values.

Also could you clarify a bit on the 'storage' file and how it's meant to work with the other parts? I tried to look through the google doc (maybe I missed it), but I didn't see a matching section. I'm assuming it's meant to globally store a persistent session of certain values, but if so - would this be compatible with the multiple/parallel auctions that Prebid.js can run; I'm wondering if there's a case of overlapping/overwriting between different auction sessions (as each auction could run its own unique adunits).

Thanks! This is looking like a good start.

Update - as another thought, are there any prebid.org docs being prepared for this new approach? If we're going to largely keep to the spec outlined in the google doc, then it should be fairly safe to prepare ahead of time while the reviews here are ongoing.

[karimMourra]

Hi @jsnellbaker thank you for the feedback! I will add inline comments/descriptions and consolidate the constants .

Not sure what you mean by the 'storage' file. Do you mean the vendorDirectory or state.js ?
State.js is meant to be a simple state object that submodules can use to store information about the current video session.
The Vendor Directory is meant to be populated at runtime by the submodules. Instead of having the core module import all the submodules, the submodule factories are added to the vendor directories.

Regarding the docs, I agree that we could start writing them, but I would prefer to wait until the code is reviewed, in case any design changes become required. I hate writing docs and I would especially hate to rewrite them.

Lastly could I get your review on jwplayer#6 ? The PR adds a layer connecting Prebid and the Video Module. It proposes a change in events.js to allow the Video events to be available without adding bloat to user who are using the module.

[lgtm-com]

This pull request introduces 1 alert when merging f474b41 into 46b1229 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging 00977b0 into ff18876 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging 6489b13 into 832c13e - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging 52b1f05 into 4af6271 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging 162630b into ca432f5 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[patmmccann]

does the gam ad server submodule need to be here?

[karimMourra]

i don't think so. The Ad Server submodule is meant to be used as a complement to a video module.
Maybe it makes sense to not mark video module as a parent in case someone wants to use it separately.

[lgtm-com]

This pull request introduces 1 alert when merging 4adc3fb into 10a3f5f - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging 32c848f into 83cb5ed - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging f63237f into 823021a - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging 84a1fc1 into 4b84596 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging 4e4d5de into aa93d86 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging ef0b22a into aa93d86 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging aef624e into 07691ce - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[lgtm-com]

This pull request introduces 1 alert when merging ff9e71a into 90239aa - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

[patmmccann]

add gam to submodules, fix lgtm alert

[lgtm-com]

This pull request introduces 1 alert when merging c1249ec into ca333b5 - view on LGTM.com

new alerts:

• 1 for Insecure randomness

[lgtm-com]

This pull request introduces 5 alerts when merging dc0026e into ff3d517 - view on LGTM.com

new alerts:

• 4 for Useless assignment to local variable
• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 4 alerts when merging 6162c0c into ff3d517 - view on LGTM.com

new alerts:

• 4 for Useless assignment to local variable

[dgirardi]

@karimMourra what do you think of reopening a new PR to cut down on the length of this thread?

[karimMourra]

@dgirardi the video.js submodule is incomplete; I will try to add the missing work and reopen! thanks!