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

Video Module: Local cache for video bids #12502

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mkomorski
Copy link
Collaborator

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Other information

#11393

@patmmccann
Copy link
Collaborator

patmmccann commented Nov 26, 2024

@mkomorski how do the blobs go away over time? Is there a risk of running out of storage?

@mkomorski
Copy link
Collaborator Author

mkomorski commented Nov 26, 2024

@mkomorski how do the blobs go away over time? Is there a risk of running out of storage?

@patmmccann They just get cleaned up when document scope is gone, so they will live till the user close the browser/tab. I don't think there is a limited memory pool dedicated for blobs so it's most likely matter of browser memory. But it can actually be a bit more efficient to also explicitly revoke not needed ones (lost bids) when auction ends up. good call

@patmmccann
Copy link
Collaborator

Maybe when the ttl expires instead? They might enter the cache

video:{"context":"outstream"}
video:{
"context":"outstream",
playerSize: [640, 360]
Copy link
Collaborator

Choose a reason for hiding this comment

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

playerSize shouldn't be required; the Video Module should be populating this, not the publisher. The Video Module is the source of truth.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mkomorski why was playerSize in all the demos, this should be populated by the Video Module already

Copy link
Collaborator Author

@mkomorski mkomorski Dec 3, 2024

Choose a reason for hiding this comment

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

@karimMourra so it seems that it's not populated for some reason, at least in all the demos. without the publisher explicitly setting playerSize to adUnit, none of the examples works. I'm not very familiar with the details of the video module, but maybe this means there is some issue? Here's the example that I was basing on - Removing playerSize from this example's code breaks it as well

Comment on lines +123 to +124
const { url, useLocal } = config.getConfig('cache') || {};
if ((!url && !useLocal) && bid.vastXml && !bid.vastUrl) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there ever a case where url would be defined and useLocal would be set to true ? Do we need a safeguard against this ?

Comment on lines +69 to +71
if (!config.getConfig('cache.useLocal')) {
url.searchParams.append(UUID_MARKER, uuid);
}
Copy link
Collaborator

@karimMourra karimMourra Dec 3, 2024

Choose a reason for hiding this comment

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

why wouldn't we append the UUID Marker when the url is local ? The idea is to know that the ad from the bid actually rendered. This is most importantly for when the bid competes in the ad server auction, but without a marker we'll never know if the ad from the bid actually rendered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that local Blob URLs can be modified in any way, adding query parameters to it will make it no longer point to the current blob and return 404. We might think of some alternative for handling this tracking in case of using local cache

src/videoCache.js Outdated Show resolved Hide resolved
src/videoCache.js Outdated Show resolved Hide resolved
mkomorski and others added 2 commits December 3, 2024 21:43
Co-authored-by: Karim Mourra <karim@jwplayer.com>
Co-authored-by: Karim Mourra <karim@jwplayer.com>
Copy link

github-actions bot commented Dec 3, 2024

Tread carefully! This PR adds 3 linter errors (possibly disabled through directives):

  • src/videoCache.js (+3 errors)

1 similar comment
Copy link

github-actions bot commented Dec 3, 2024

Tread carefully! This PR adds 3 linter errors (possibly disabled through directives):

  • src/videoCache.js (+3 errors)

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.

4 participants