Bug in renderAd for video

[EskelCz]

Type of issue

bug

Description

Prebid.js/src/prebid.js

Line 532 in 6d39d8b

const adUnit = pbjsInstance.adUnits.filter(adUnit => adUnit.code === adUnitCode);

On line #532 it does array.filter, which always returns a new Array. Then at #534 it looks for .video on that array. Since the variable is called adUnit, I would guess it's supposed to take the first index of the array.

Steps to reproduce

Set up a video adUnit and have a winning bid.

Test page

https://playground.cpex.cz/combination

Expected results

Call videoModule.renderBid and hopefully render something :) Not sure what follows then, I'm in middle of figuring out how to do instream ads.

Actual results

ERROR: Error trying to write ad. Ad render call ad id <adId> was prevented from writing to the main document.

Platform details

Mac OS, latest Chrome, Prebid 8.12.9

[karimMourra]

Hi @EskelCz I took a quick look at the cpex-package.js file in the demo page that you shared, and it doesn't seem like you've configured your prebid instance to use the Video Module. Perhaps I am looking at the wrong file.
If you're not familiar with how to configure the Video Module I suggest looking at the demo examples we have in https://github.com/prebid/Prebid.js/tree/master/integrationExamples/videoModule - we have demos using both JW Player and Video.js . For the most basic example I suggest https://github.com/prebid/Prebid.js/blob/master/integrationExamples/videoModule/jwplayer/eventListeners.html

If you have properly configured Prebid to use the Video Module please let me know what file to look at in your demo page and I'll see if I can help

[EskelCz]

@karimMourra Thanks for checking the demo page, you got the right file and you're correct, I missed the video module configuration. However, I think the bug I'm describing still holds, just based on the data types I described. One function returns an array of adUnits, the second expects a single adUnit object.

Regarding the video object configuration, I'm perplexed by the divId: 'player' setting. I need the target elements to be dynamic, based on which adUnits are set for the current page and which wins the auction. At minimum, I need an array of elements. Is there a way around it?

[dgirardi]

@karimMourra; if pbjs.renderAd is not intended to be used with the video module, can we remove this?

Prebid.js/src/prebid.js

Lines 529 to 538 in 6d39d8b

	// video module
	if (FEATURES.VIDEO) {
	const adUnitCode = bid.adUnitCode;
	const adUnit = pbjsInstance.adUnits.filter(adUnit => adUnit.code === adUnitCode);
	const videoModule = pbjsInstance.videoModule;
	if (adUnit.video && videoModule) {
	videoModule.renderBid(adUnit.video.divId, bid);
	return;
	}
	}

otherwise, what should it do? beside the problem in the OP, there's also the issue that the adunit is not necessarily in the global pbjs.adUnits array.

[karimMourra]

@EskelCz you're right, it should access the element in the array; we will fix that.
As far as the divId setting, the Video Module needs to know what player instance to hook into. A page could have multiple video players embedded in it. By specifying the HTML Div Id of the player, we are able to target the specific instance that you would like to use. Also different ad units can be paired to different video player instances on the page.
Could you elaborate on your use case ?

[EskelCz]

@dgirardi I feel like I'm missing part of the conversation. renderAd is not supposed to be used for video? I'm increasingly confused about the Prebid rendering pipeline. Is it similar to the issue with Native format and the renderAd function, we discussed here? #10006 (comment)

---

@karimMourra Ok lets take this example: https://github.com/prebid/Prebid.js/blob/master/integrationExamples/videoModule/jwplayer/eventListeners.html

There is a divId key in the adUnit and then there is a divId key in the video module config.

1. Imagine a page with 5 video adUnits, each has it's own divId. How do I configure the video module? I see that I can only set one divId per provider. Do I duplicate the providers? That would be even more redundant.
2. Why can't prebid take the divId from the adUnit? To me it looks redundant and makes it harder to configure, in a dynamic system that we have.

[dgirardi]

Some confusion is justified. renderAd has a specific exclusion for video, afaik since the beginning:

Prebid.js/src/prebid.js

Lines 550 to 552 in 5a1ebad

	} else if ((doc === document && !inIframe()) || mediaType === 'video') {
	const message = `Error trying to write ad. Ad render call ad id ${id} was prevented from writing to the main document.`;
	emitAdRenderFail({reason: PREVENT_WRITING_ON_MAIN_DOCUMENT, message, bid, id});

which makes some sense (except for the error message) since video needs a separate player to be set up which Prebid didn't attempt to do.

The (relatively) new video module does more player management, and knows how to render, but from my understanding its philosophy is that once an adUnit is configured for it nothing else is needed - the module will take care of everything including rendering. Hence why I'm not sure of the scenario where you'd need to renderAd.

[karimMourra]

@EskelCz

Why can't prebid take the divId from the adUnit? To me it looks redundant and makes it harder to configure, in a dynamic system that we have.

The Video Module needs a provider to be listed in the config in order to attach itself to the video player. If the player is not instantiated, it instantiates the video player at this point; the configuration is provided by the publisher in the provider config. You only need to register a provider for every video player instance that you expect to use. If you're using one player for several ad units, you only need to register once. The relationship is 1 to many.

Imagine a page with 5 video adUnits, each has its own divId. How do I configure the video module? I see that I can only set one divId per provider. Do I duplicate the providers? That would be even more redundant.

So on your page you have 5 different video player instances ? In that case you would need 5 video providers in your config. You can call setConfig multiple times, whenever you need to add a video provider.

If you have 5 video players on the page, then why are you concerned about redundant providers ? 5 video players is a lot of overhead already.

The Video Module is meant to do more than render ads: it emits video and media events that publishers can use for analytics or for scheduling ads. It populates the video block in the ad unit on behalf of the publisher, populates the content metadata in the ortb fragment for the bid request, in both cases using accurate information. It also simplifies integration with Video Players by managing the player's lifecycle on behalf of the publisher. In order to achieve this, we believe that it's best for the player to be attached as early as possible. This was designed with mostly instream use cases in mind; we will be adding better outstream support in the near term.

When you see room for improvement, we welcome you to implement features that would help your usecase. It would be helpful to us if you could explain your dynamic usecase a bit more. Are you using JW Player or Video.js ?

[EskelCz]

@karimMourra I see, I didn't realize there might be multiple adUnits per player. I was in a mindset of regular banner ads with a simple 1:1 relation.

Regarding multiple players, it's probably not going to happen often, but since we're generating our config and adUnits for many different publisher use-cases, I wanted our abstraction to be most versatile and simple to implement. But it's ok, multiple "providers" is workable I guess. Thanks for the clarification.

However I still don't understand the rendering flow. We're sending the returned bids to a custom adserver, it returns winning bids and has to somehow trigger the render. For banner ads that is with the pbjs.renderAd method. Is there an equivalent method for the instream video?

[karimMourra]

@EskelCz I fixed the issue you reported regarding the array in #10567 . I also updated the example page in that PR to use renderAd . Could you please take a look and let me know if that works for your use case ?
WHat does your custom ad server return ? An ad tag or a json representation of the winning bids ?

[EskelCz]

@karimMourra An html with a javascript tag, I guess what you call an ad tag.
Thanks for the fix, I think that's just what we need.

I'm testing it with your PR version and there's one more issue though. For some reason the adUnit from adUnit = auctionManager.index.getAdUnit(bid) is missing the video key, therefore it fails to render properly, this time without the error. The video key is there when I call pbjs.adUnits. You can see it in the test page.

[EskelCz]

at the same time:

[karimMourra]

@EskelCz it seems like you're comparing 2 different ad Units; I can tell because the adUnit's code is different.
In one case it's rectangle-1 , in the other it's preroll-1, where rectangle-1 doesn't seem to have the video key.

[EskelCz]

@karimMourra Sorry you're right. It triggers the videoModule.renderBid seemingly with the right data, but the player behaves in a strange way. It first shows the content video, and once the auction starts it turns black. I'll check the video.js docs.

[karimMourra]

We have a folder with example pages for video.js in https://github.com/prebid/Prebid.js/tree/master/integrationExamples/videoModule/videojs perhaps one of these can be useful

[EskelCz]

@karimMourra It was a struggle but it works :) Thanks a lot

[EskelCz]

@karimMourra Found one additional issue with the video module. When the divId in video.providers or mediaTypes.video doesn't match, the rendering fails without any error or warning. Just took me a few hours to figure out the discrepancy because it failed silently.
Also, any ETA on the release of the bugfix? Thanks

[patmmccann]

@EskelCz could you open the new issue as a different one to track

[EskelCz]

@EskelCz Sure, but I'll leave this one open, since the PR with the fix wasn't merged yet.

[dgirardi]

This should have been fixed with #10819

[EskelCz]

Thanks, I'll get back to it later. Meanwhile closing this issue.