Conversation
The rendering BannerView could only request a single format: its constructors set AdFormat.BANNER and setVideoPlacementType() calls setAdFormat(VAST), which clears banner (video-only). So a rendering-API banner placement couldn't let banner and outstream video compete on one impression — unlike InterstitialAdUnit (EnumSet<AdUnitFormat>) and the original API. Add setAdUnitFormats(EnumSet<AdUnitFormat>) and setVideoParameters(...) to BannerView, delegating to the already-public AdUnitConfiguration API. No renderer changes: BannerView -> DisplayView -> PrebidRenderer.createBannerAdView -> PrebidDisplayView already branches on BidResponse#isVideo() (VideoView vs banner); only the request-side format was gated. Intent to implement: prebid#999 Co-authored-by: Claude <noreply@anthropic.com>
Addresses review feedback: setVideoParameters() on a rendering BannerView was inert beyond video.w/h. BasicParameterBuilder.setVideoImpValues() reads the full VideoParameters (mimes, protocols, playbackmethod, api, plcmt, placement, durations, bitrates, skippable, battr) only inside if (isOriginalAdUnit()) — a flag no rendering-API class sets — so a multiformat BannerView's outstream imp shipped hardcoded defaults and placement=INTERSTITIAL. Relax the gate to also read VideoParameters when explicitly set: if (adConfiguration.isOriginalAdUnit() || adConfiguration.getVideoParameters() != null) Guarded — rendering units that never call setVideoParameters keep the original default branch, so existing behavior is unchanged. Also resolves the placement concern: params carry placement/plcmt, so no separate setter is needed. isOriginalAdUnit is intentionally not flipped (it drives more than video imps).
|
Thanks for the thorough review — both points were spot-on. Pushed a follow-up commit taking option (a).
if (adConfiguration.isOriginalAdUnit() || adConfiguration.getVideoParameters() != null)It's guarded on Non-interstitial Happy to add tests or adjust the approach (e.g. a dedicated non-original branch instead of the |
testRenderingApiVideoParameters_{interstitial,nonInterstitial}_full both
assert video.placement == 5 (Interstitial) — the value the rendering path
always hardcoded before this PR's fix. Both tests call
configuration.setVideoParameters(createFullVideoParameters()), which sets
placement = InBanner (2) — exactly the value the ORIGINAL-API path already
asserts for the same input (see the existing assertEquals(new Integer(2), ...)
a few tests up). With the isOriginalAdUnit() || getVideoParameters() != null
gate this PR adds, the rendering path now reads that same explicit value
instead of defaulting to Interstitial, so these two fixtures need updating to
match — the code change is correct, the tests were stale.
Confirmed testRenderingApiVideoParameters_nonInterstitial_empty (no
VideoParameters set) is unaffected and correctly keeps placement == 5: that
test's config has getVideoParameters() == null, so the new gate still takes
the pre-existing default branch for it.
Could not run this locally (AGP requires JDK 17; only JDK 11 available in
this environment) — verified the expected value by cross-referencing the
already-passing original-API assertion for the identical input, not by
executing the suite. Please confirm on CI.
Intent to implement
#999
Problem
The rendering
BannerViewcan only request a single format:AdFormat.BANNER, andsetVideoPlacementType(...)callsadUnitConfig.setAdFormat(VAST), which clears banner → video-only.So a rendering-API banner placement can't let banner and outstream video compete on one impression — unlike
InterstitialAdUnit(context, configId, EnumSet.of(AdUnitFormat.BANNER, AdUnitFormat.VIDEO))and the original API (#610). Requested for banner in #621, where only interstitial was addressed.Change
Add two passthroughs to
BannerView, delegating to the already-publicAdUnitConfigurationAPI (the same oneInterstitialAdUnituses):Why no renderer changes are needed
BannerView → DisplayView → PrebidRenderer.createBannerAdView → PrebidDisplayView, andPrebidDisplayViewalready branches onBidResponse#isVideo()(VideoViewfor VAST, banner otherwise). Only the request-side format set was gated; the render path already handles the winning format.Notes
BannerView, which already exposesadUnitConfig.adFormats— this closes an Android/iOS asymmetry.EnumSet<AdUnitFormat>constructor instead of setters, per maintainer preference — kept this minimal for PMC review of Intent to implement: multiformat (banner + outstream video) on the rendering BannerView #999 first.Disclosure: implemented with AI assistance (Claude Code); reviewed and submitted by @nsp37.
🤖 Generated with Claude Code