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

PBjs Core: allow video cacheKey for outstream #8833

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

renebaudisch
Copy link
Contributor

Type of change

  • Feature

Description of change

let seller add an option "useCacheKey" to mediaTypes.video definitions to optional allow video cacheKey for outstream

@ChrisHuie ChrisHuie changed the title allow video cacheKey for outstream PBjs Core: allow video cacheKey for outstream Aug 15, 2022
@ChrisHuie ChrisHuie requested a review from dgirardi August 15, 2022 16:56
@ChrisHuie ChrisHuie requested a review from patmmccann August 15, 2022 16:56
export const checkVideoBidSetup = hook('sync', function(bid, adUnit, videoMediaType, context) {
if (videoMediaType && context !== OUTSTREAM) {
export const checkVideoBidSetup = hook('sync', function(bid, adUnit, videoMediaType, context, useCacheKey) {
if (videoMediaType && (useCacheKey || context !== OUTSTREAM)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am missing some context - why were we disabling video cache for outstream? I'm trying to understand if it'd make more sense to have this as a global toggle that says "enable caching for all video", rather than a flag for each adUnit. maybe setConfig({cache: {outstream: true}})?

If we do a setting at the adUnit level:

  • I think cache or useCache would be a better name than useCacheKey
  • do we need to keep the distinction between outstream and the rest? that is, could this say if (videoMediaType && useCache), with useCache defaulting to false for outstream, and true otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know why this is coded for instream only, but I use this in my fork for month right now, so I can pass a dynamic url to a vast wrapper for counting. Maybe earlier this was thought to be needed as you always needed a "renderer" to setup outstream? I don't know, but to not break other things I thought it would be good to keep the actual state and only add a new option.

I'm also fine to set this as a global option, maybe even have both for most flexibility, while adUnit setting would overwrite the global.

And if we go on here I will be pleased to change useCacheKey to whatever you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We tried to investigate and it's unfortunately still unclear why we originally decided to not cache outstream. @jsnellbaker do you happen to remember some context? it seems that outstream caching was disabled in MikeSperone@f272031#diff-e9f987f170d6e2065c627eb6a145180770dc0921cb2c82f0c0a549ae87934a4dR363

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found this:
#3024 (comment)
and this:
#6078

but would adding using cache for outstream as an optin to the pageOptions affect this?
I do need the cache to convert a xml response to an url to pass it as a query parameter (to our adserver).

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I always understood, video caching was utilized for setups where a video bid was being fed into a player integrated onto the page, since you didn't know when exactly the video bid was going to be rendere (ie pre-roll, mid-roll, post-roll, etc). I may be mistaken (or out of date), but I think there was some ttl issues where the video bid could expire if it was directly accessed from the prebid.js window instance from the player. So this led to having the vast creative info being cached (with a longer shelf-life) and pulled later at the player's convenience.

Outstream though is generally assumed to A) come with its own renderer (either supplied with the bid itself from the bidder's adserver or from the publisher setup in the adUnit) and B) the ad experience is simpler than instream (where you may just see the ad in the outstream player and that's it). Additionally, outstream placements generally have their own dedicated section on the page, much like any other banner placement - so the ad could be rendered immediately as soon as the publisher's adserver (eg GAM) decided to choose that outstream prebid bid. Therefore, there wasn't a need to cache things - since the bid information would typically be used asap and it had the means to do its rendering on its own.

This is my understanding - which I was not an original author on the initial outstream/instream workflow for prebid and I only learned from some others as they knew it.

All that being said, as for why we can't use caching with outstream - the above basically distills to a 'it wasn't done like that before' (at least that's what I see looking over it again). If there are needs now to support it - I'm not opposed to the changes and there may be a way to make things 'work', though there would need to be extensive thought and testing to handle the old-way in conjunction with this new way - since I don't think most outstream setups would support caching or see a need to move in that direction (unless there were strong advantages).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer simplicity, and that would be enough of a reason for me to use caching without introducing a distinction between instream and outstream. Maybe it's not often necessary but that doesn't seem enough of a reason to forbid it in code. It sounds like you are in that situation @renebaudisch? You have a player set up to work with cache URLs and it'd be nice to not have to set up a whole other process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a video setup that works with urls and expects a xml vast response. It is used on the floating videoplayer e.g. on this url: https://www.computerbild.de/artikel/cb-Ratgeber-Software-kostenloser-Virenschutz-1153794.html

First we're using prebid. The winner get's passed to xandr for the rtb auction done by a ptv request.
So, if now the prebid bid returns as a winner, the player will need a xml wrapper containing an url coming from the adserver as response, this is where the cacheKey url comes into play.

As we need to host this response in Xandr as a video creative we cannot return the prebid xml by ourselves, even if we would pass it through. And also, as the vast plugin already is working, we also cannot interfere here and replace it.

Sure, we also could change the context to instream, but as this is whether in- nor outstream and we need a way to separate small view from fullview to the bidders, I'm back at the point where I need the cacheKey url...
Because, the small view is sending outstream context while the fullview on click sends instream to show the bidder what they are bidding for.

And since my change is backwards compatible I though why not submit it.


if (config.getConfig('cache.url') && context !== OUTSTREAM) {
if (config.getConfig('cache.url') && (useCacheKey || context !== OUTSTREAM)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think useCache would be a better name for the mediaTypes.video setting - and I think this doesn't need to check if context is outstream; let's just allow it on every video adUnit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be clear - I'm suggesting changing this to say "use the cache if mediaTypes.video.useCache is true", which should default to true if missing, except for outstream (just to keep current behavior). So that instead of adding a special case on a special case, we could some day potentially remove this distinction entirely (for caching).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this parameter perhaps be a bidderSettings option?

If we're setting this value in the mediaTypes.video object and you had multiple bidders within the same outstream adUnit - how would it work if one bidder used the cache approach and one didn't? The adUnit shares the same setting if it's coming from the mediaTypes.video - so it may use the cache approach when it may not work for a bidder's outstream setup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell, caching means updating a video bid with videoCacheKey and vastUrl only if it's missing. if a bid comes with a renderer that doesn't know how to use them, they shouldn't hurt? Do bidders care about the cache for the instream case? it seems backwards to me that they would.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bidders I don't think would care about prebid cache since it just happens automatically (from their perspective) or not - as the rendering is handled by the publisher's video player setup. Side note - Though I'm not sure publishers really would elect to not configure prebid cache if they were running instream, since the docs point that way strongly and its their benefit for the ad experience.

But outstream is different in only that the bidder has something of a hand to work with the renderer interface we have setup in Prebid.js - to preconfigure the renderer and setup the call that starts the process. So if the process/workflow changes unexpectedly, that may cause some issues. It would depend on the situation and the changes are implemented. It may be fine in the end with what's being planned here, but wanted to raise the thought and ensure it gets checked as part of the review here (to avoid side-effects/edge-cases).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will try to test it out soon and follow-up with the results from our setup. Those results may not cover all the outstream setups out there, but it may give some idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I setup a test page with the flag here enabled and prebid cache url defined in the config. The outstream ad (though it went through the caching process) rendered on its own fine. This is because of how the appnexus bidder grabs the video bid information; it uses the raw bid response from the endpoint and passes it to the renderer to process/load onto the page. So this outstream workflow doesn't utilize the updated vastUrl or videoCacheKey params in the prebid bid object, and thus seems to be okay with these changes (from what I can tell).

Copy link
Collaborator

@patmmccann patmmccann Aug 18, 2022

Choose a reason for hiding this comment

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

@renebaudisch It seems like the most popular rendering workflow doesn't take advantage of the cache after this change. Is that expected by you? If so, how does this help you? Do you have a different rendering workflow in mind?

Does this only work with non-multiformat outstream units?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @patmmccann. I explained our bidflow above.

Also, my intension is to not change anything automatically, the change is meant to offer a new option that has to be activated by intension, at least that was my intension. If the decision concludes to always just useCacheKey my case would work either so I won't stop you but also not please you for this, but I just wanted an additional option that I use in my fork to be available in the official build so I do not have to patch my fork on every prebid update we do.

And I haven't testet multiformat adunits, but they should work as before, we do not change the flow here, we just update something inside the video processing so this shouldn't be able to affect native or banners.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks!

}

export const checkVideoBidSetup = hook('sync', function(bid, adUnit, videoMediaType, context) {
if (videoMediaType && context !== OUTSTREAM) {
export const checkVideoBidSetup = hook('sync', function(bid, adUnit, videoMediaType, context, useCacheKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need the additional parameter - you already have videoMediaType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but videoMediaType is true for in- and outstream. I just want to add a switch to outstream, so I have to pass the value of that switch.

We have instream where cacheKey always needs to be true and outstream where now it always is false, both with videoMediaType set to true, so passing useCacheKey only respected for outstream seems to me the only way here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean you can move line 38 here (useCacheKey = videoMediaType && deepAccess(videoMediaType, 'useCacheKey')), since videoMediaType is the same object.

@patmmccann
Copy link
Collaborator

Thanks @renebaudisch ! Are you able to document this flag as well on the docs project?

@patmmccann
Copy link
Collaborator

@fowler446 curious if you could suggest the best docs location?

@fowler446
Copy link
Collaborator

@patmmccann
I think this is the best location:
https://docs.prebid.org/dev-docs/adunit-reference.html#adUnit.mediaTypes.video

Also, adding another Outstream example with caching would be good too:
https://docs.prebid.org/dev-docs/adunit-reference.html#outstream

renebaudisch pushed a commit to renebaudisch/prebid.github.io that referenced this pull request Aug 22, 2022
@renebaudisch
Copy link
Contributor Author

How about this?
prebid/prebid.github.io#3969

@fowler446
Copy link
Collaborator

@renebaudisch
Yup, that looks good 👍.

@patmmccann patmmccann merged commit 2b1c60e into prebid:master Aug 31, 2022
fowler446 pushed a commit to prebid/prebid.github.io that referenced this pull request Sep 3, 2022
Co-authored-by: René Baudisch <rene.baudisch @axelspringer.com>
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
ecdrsvc pushed a commit to ecdrsvc/prebid.github.io that referenced this pull request Sep 6, 2023
Co-authored-by: René Baudisch <rene.baudisch @axelspringer.com>
jlaso pushed a commit to AuDigent/prebid.github.io that referenced this pull request Nov 6, 2024
Co-authored-by: René Baudisch <rene.baudisch @axelspringer.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants