Renderer method used on all mediaType

[jdelhommeau]

Type of issue

Bug

Description

When running multiple mediaType on a single adUnit (for example: outstream + banner) and you need to use client side renderer for the video outstream, the renderer will also try to render banner ad, not just outstream ad. Ideally, there is a way to either have a way in renderer to fallback on default renderer, OR the renderer is only called for video ad returned.

Steps to reproduce

1 - Build an adUnit for video and banner mediaType that delivers both video and banner creative
2 - define manual renderer
3 - run. When a banner ad is returned, the rendering fails because of missing information (bid.adResponse is missing on banner)

Test page

http://jdelhommeau.devnxs.net/POC/VIDEO/prebid_outstream_banner_plct.html

Expected results

Way to fallback on default renderer when in renderer method OR renderer is only used when video mediatype is returned

Actual results

Renderer is called all the time for all mediaType and fails when not video

Platform details

Other information

[benjaminclot]

@jdelhommeau
You could do something like this:

pbjs.onEvent("bidWon", function(bid) {
  if (/* condition */) {
    bid.renderer = {
      url: rendererUrl,
      render: rendererFn
    };
  }
});

[pkayfire]

Any updates here? This is a pretty big issue with anyone that wants to run a multi-format placement with banner + outstream video.

@benjaminclot not sure the workaround works 100% of the time. In my case the renderer URL did not load.

[benjaminclot]

@pkayfire I use a fake URL as rendererUrl because my rendererFn is already available. It's just that renderer requires a URL parameter.

[jaiminpanchal27]

Here is possible solution to this bug:
Renderer module is not tied to any particular format, even though it is only used for outstream by Prebid adapters as of now.

Prebid core installs renderer on bid response if renderer is present on adUnit. See here

We can add renderer property inside mediaTypes.<format> object in case of multi-format. By doing this Prebid core will be able to find out for which format it needs to install renderer.

This will not be a breaking change, as we will support existing feature as well where renderer is directly added at the topmost level in adUnit. Maybe we can remove that in 3.0

@mkendall07 @bretg Your thoughts.

[mkendall07]

what if we just associated a renderer with every bid, and for banner the renderer is simply the "renderAd" function that is standard for banner?

[jdelhommeau]

I really like @jaiminpanchal27 idea of having ability to assign renderer per mediaType.
That would also allow people to add renderer for native (when not using an adserver for example) or to add renderer for specific banner case (skin).
We would also need to have a way in the custom renderer function to passback to default renderer from prebid. That way, you could have stuff like:

renderer for display: function(bid){

if (bid.width == 1800 and bid.height == 1000) { // skin case
// apply css / js code for correct rendering of skin creative
}
defaultRendererFunction();
}

That would be a huge improvement over current way of implementing such format.

[pkayfire]

For the sake of speed it could make sense to go with the solution by @mkendall07 and have the renderer per mediaType be a follow-up feature.

[Hamper]

Multiformat auctions still can't be used with custom renderer?

[jdelhommeau]

@pkayfire and @mkendall07 was there any updates on this ticket? Has support for different renderer per mediaType been improved?

[patmmccann]

So if a publisher were to define types in the ad unit renderer like this

pbjs.addAdUnit({ code: 'video1', mediaTypes: { video: { context: 'outstream', playerSize: [640, 480] } }, renderer: { url: 'https://acdn.adnxs.com/video/outstream/ANOutstreamVideo.js', types: ['video'], render: function (bid) { ANOutstreamVideo.renderAd({ targetId: bid.adUnitCode, adResponse: bid.adResponse, }); } }, ... });

and https://github.com/prebid/Prebid.js/blob/master/src/auction.js#L515 was modified from if (adUnitRenderer && adUnitRenderer.url) { bidObject.renderer = Renderer.install({ url: adUnitRenderer.url }); bidObject.renderer.setRender(adUnitRenderer.render); } to

if (adUnitRenderer && adUnitRenderer.url && !(adUnitRenderer.types && adUnitRenderer.types.indexOf(bidObject.mediaType) !== -1)) { bidObject.renderer = Renderer.install({ url: adUnitRenderer.url }); bidObject.renderer.setRender(adUnitRenderer.render); }

would that work?

[patmmccann]

plan is to implement the types check after #5638 gets merged so as to not introduce a merge conflict

[MikeSperone]

Not sure if there's a use case for this - but what if you also needed a separate renderer for display or native (or both)?
@patmmccann would your solution allow for both separate 'banner' renderer and a 'video' renderer?

I was thinking the publisher could define it like this, and it could potentially cover additional use cases:

pbjs.addAdUnit({
    code: 'video1',
      // This renderer would work the same as it does now...
    renderer: {
        url: 'example.com/publishersCustomRenderer.js',
        render: function(bid) { renderAdUnit(...) }
    },
    mediaTypes: {
        video: {
            context: 'outstream',
            playerSize: [640, 480],
              // but a renderer passed in here would apply only to this mediaType.
              // It would override the above renderer if that was defined. ...
            renderer: {
                url: 'example.com/videoRenderer.js',
                render: function (bid) { renderVideo(...) }
            }
        },
        display: {
            ...,
              // and you'd be able to do the same for each mediaType
            renderer: {
                url: 'example.com/displayRenderer.js',
                render: function(bid) { renderDisplay(...) }
            }
        }
    },
    ...
});

In which case, I think you could do something like this:

var renderer = null;

var mediaTypeRenderer = !!bidReq[bidObject.mediaType] && bidReq[bidObject.mediaType].renderer;
if (!!mediaTypeRenderer && mediaTypeRenderer.url && mediaTypeRenderer.renderer) {
    renderer = mediaTypeRenderer;
} else if (adUnitRenderer && adUnitRenderer.url && !(adUnitRenderer.backupOnly && isBoolean(adUnitRenderer.backupOnly) && bid.renderer)) {
    renderer = adUnitRenderer;
}

if (renderer) {
    bidObject.renderer = Renderer.install({ url: renderer.url });
    bidObject.renderer.setRender(renderer.render);
}

Also, I do actually need this feature (either your proposed solution or this one would work for me)... so if you're not already working on this, I'd be happy to create a PR for this

[patmmccann]

having up to three different renderers on a given ad unit, one for each mediatype, is a much better solution than the type check i am proposing, but we don't want the perfect to be the enemy of the good and that solution is much harder.

[hnkhandev]

Could a temporary fix (until the type check is implemented and merged) be something like just passing the bid through and checking if the mediaType is video in isRendererRequired?

Prebid.js/src/Renderer.js

Line 100 in 466b49e

export function isRendererRequired(renderer) {

[MikeSperone]

@patmmccann I actually think I got something that might work - it seems to be working so far. I can create a PR, or first paste a link to it in my fork when it's ready.

[patmmccann]

Awesome, what have you got

…

On Fri, Sep 11, 2020, 2:52 PM Mike Sperone ***@***.***> wrote: @patmmccann <https://github.com/patmmccann> I actually think I got something that might work - it seems to be working so far. I can create a PR, or first paste a link to it in my fork when it's ready. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3231 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM25Z2FKRPNKR5TV2BRFSDSFJWYTANCNFSM4F7KKG5A> .

[MikeSperone]

@patmmccann I want to add a test case still, but this is working for me - and the existing tests still pass

MikeSperone@7b7dad6
(edit: had to make another small change)
https://github.com/prebid/Prebid.js/compare/master...MikeSperone:mediaType-renderers?expand=1

[MikeSperone]

I'm not sure if I should make a new issue or ask this here, but I am trying to write a test for my above change, and placing my test after the 2 tests here: https://github.com/prebid/Prebid.js/blob/master/test/spec/auctionmanager_spec.js#L721-L764

I'm confused about the video-outstream media type, which I can only find reference to in the tests and not anywhere in the code or documentation.
If I use "video-outstream" as the mediaType, as in L757 then I can get a value for addedBid. But the type needs to be video since I am testing adding a renderer to the "video" mediaType. I don't understand why a mediaType of video-outstream seems to be required to receive bids in this test if in the real-world it seems this value should be video for a multiFormat video

[bretg]

@MikeSperone - there's not really such a thing as a mediatype called video-outstream. The mediatype is video and the context is outstream. Unit tests can use test strings that are different than production use.

The one place the video-outstream string exists is in the mediatypepricegranularity config, and that's a hack shortcut.

[MikeSperone]

Thanks @bretg! That was very helpful! I found out that my test was actually working, it was my code that had the error

[patmmccann]

I think this was solved by 5760 and can be closed

[ChrisHuie]

Solved with #5760