Add support for video stream context #1483

matthewlane · 2017-08-14T15:14:15Z

Type of change

Feature

Description of change

This pull request adds support for video-enabled adapters to participate on both instream and outstream impressions by removing the video-outstream mediaType and introducing a new ad unit parameter mediaTypes that can be used to configure video ad units to either instream (default) or outstream. No changes are required for adapters that support the current video mediaType, though the mediaTypes parameter of an adUnit, which may contain a stream context, is now given to adapters in the callBids parameter, and adapters may use this to target instream and outstream impressions differently if they wish.

In the case of instream context, bid response objects must contain a vastUrl property (this is an existing requirement and not introduced by this PR). Outstream bid response objects must have an associated renderer, either directly on the bid or one defined on the ad unit with the renderer ad unit parameter.

Example AdUnits with Video Context

pbjs.addAdUnits({
  code: 'video-example',
  sizes: [640, 480],
  mediaTypes: {
    video: { context: 'instream' }
  },

  bids: [{
    bidder: 'appnexusAst',
    params: { placementId: '9333431' }
  }]
});

pbjs.addAdUnits({
  code: 'video-example',
  sizes: [ 640, 480 ],
  mediaTypes: {
    video: { context: 'outstream' }
  },

  // appnexusAst contains a renderer so this isn't required, but if
  // present it will be used in favor of the response renderer
  renderer: {
    url: 'http://cdn.adnxs.com/renderer/video/ANOutstreamVideo.js',
    render: function(bid) {
      ANOutstreamVideo.renderAd({
        targetId: bid.adUnitCode,
        adResponse: bid.adResponse
      });
    }
  },

  bids: [{
    bidder: 'appnexusAst',
    params: { placementId: '5768085' }
  }]
});

Other informatin

@unrulydevelopers unruly adapter is updated by this PR to check for outstream context, please feel free to review and make updates or suggestions to this change

jaiminpanchal27

@matthewlane left some comments.

jaiminpanchal27 · 2017-09-05T16:14:16Z

src/utils.js

+ * @param{MediaTypes} mediaTypes mediaTypes parameter to validate
+ * @returns{boolean} If object is valie
+ */
+export function mediaTypesValid(mediaTypes) {


For all functions returning boolean, naming should start with is or has. I was going through it over here https://google.github.io/styleguide/jsguide.html#naming-method-names.
So this function can be renamed as hasValidMediaType or isValidMediaType

jaiminpanchal27 · 2017-09-05T16:15:56Z

src/video.js

+/*
+ * Validate that the assets required for video context are present on the bid
+ */
+export function videoBidIsValid(bid) {


Comment made above for function mediaTypesValid applies here too.
Also add missing JSDoc for param and return value and unit test for this function

jaiminpanchal27 · 2017-09-05T16:22:04Z

src/video.js

+
+  // if context not defined assume default 'instream' for video bids
+  if (!bidRequest || !context || context === INSTREAM) {
+    return !!(bid.vastUrl || bid.vastPayload);


We are removingbid.descriptionUrl when prebid-cache is live or stable.
So on that note, will this also include bid.descriptionUrl ? Not very sure though.

It won't, just bid.vastUrl or a vast payload is required. vastPayload was renamed vastXml, will push an updated to fix that here

jaiminpanchal27

LGTM

mkendall07

Looks great @matthewlane nice work! Few minor things and we can merge.

mkendall07 · 2017-09-15T12:42:40Z

src/adaptermanager.js

+          if (utils.isValidMediaTypes(adUnit.mediaTypes)) {
+            bid = Object.assign({}, bid, { mediaTypes: adUnit.mediaTypes });
+          } else {
+            utils.logError(`mediaTypes is not correctly configured`);


can you add the adUnit.code here so they know which adUnit failed validation?

mkendall07 · 2017-09-15T12:45:35Z

test/spec/video_spec.js

+
+    expect(valid).to.be(true);
+
+    utils.getBidRequest.restore();


these calls are better in a afterEach block so failures don't cascade.

…built * 'master' of https://github.com/prebid/Prebid.js: (46 commits) Serverbid alias (prebid#1560) Add user sync to process for approving adapter PRs (prebid#1457) fix travis build (prebid#1595) Rubicon project improvement/user sync (prebid#1549) Adding Orbitsoft adapter (prebid#1378) Fix renderer test for new validation rule (prebid#1592) Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577) Add support for video stream context (prebid#1483) Invalidate bid if matching bid request not found (prebid#1575) allow adapters to set default adserverTargeting for specific bid (prebid#1568) Custom granularity precision should honor 0 if it is passed in closes prebid#1479 (prebid#1591) BaseAdapter for the Prebid 0.x -> 1.x transition (prebid#1494) Add a version to the Criteo adapter (prebid#1573) Allow bundling from node.js or with new gulp task bundle-to-stdout (prebid#1570) Add url.parse option to not decode the whole URL (prebid#1480) Tremor Video Bid Adapter (prebid#1552) Yieldmo bid adapter (prebid#1415) Switch `gulp docs` to build its output using documentation.js (prebid#1545) Increment pre version Prebid 0.28.0 Release ...

* Add support for video stream context * Define adapter as supporting video * Use mediaTypes param to specify context * Use utils.deepAccess * Check for outstream bids * Add JSDoc and validation * Rename functions and add unit test * Update property name * Update stubs to new sinon stub syntax * Only check context when mediaTypes.video was defined * Retain video-outstream compatibility * Revert to Sinon 1 syntax * Server and bid response ad type for any stream type is always 'video' * Update to address code review

* tag '0.29.0' of https://github.com/prebid/Prebid.js: (29 commits) Prebid 0.29.0 Release Fix for not syncing bidders. (prebid#1598) fix amp example pages (prebid#1597) closes prebid#1298 (prebid#1583) Fixed the broken tests. (prebid#1602) Rubicon Bidder Factory (prebid#1587) Trustx adapter (prebid#1488) Add nurl to markup (prebid#1601) Pass bidRequest to createBid (prebid#1600) Add Kumma adapter (prebid#1512) Serverbid alias (prebid#1560) Add user sync to process for approving adapter PRs (prebid#1457) fix travis build (prebid#1595) Rubicon project improvement/user sync (prebid#1549) Adding Orbitsoft adapter (prebid#1378) Fix renderer test for new validation rule (prebid#1592) Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577) Add support for video stream context (prebid#1483) Invalidate bid if matching bid request not found (prebid#1575) allow adapters to set default adserverTargeting for specific bid (prebid#1568) ...

* Add support for video stream context * Define adapter as supporting video * Use mediaTypes param to specify context * Use utils.deepAccess * Check for outstream bids * Add JSDoc and validation * Rename functions and add unit test * Update property name * Update stubs to new sinon stub syntax * Only check context when mediaTypes.video was defined * Retain video-outstream compatibility * Revert to Sinon 1 syntax * Server and bid response ad type for any stream type is always 'video' * Update to address code review

dbemiller requested a review from mkendall07 August 14, 2017 16:24

dbemiller assigned mkendall07 Aug 14, 2017

mjacobsonny added the video label Aug 17, 2017

matthewlane force-pushed the feature/video-stream-context branch from fa34232 to 63f5c19 Compare August 28, 2017 18:18

matthewlane added the in progress label Aug 28, 2017

matthewlane force-pushed the feature/video-stream-context branch 2 times, most recently from b9df58c to 3d89da0 Compare August 30, 2017 17:15

jaiminpanchal27 requested changes Sep 5, 2017

View reviewed changes

matthewlane force-pushed the feature/video-stream-context branch from 111d829 to 7774e78 Compare September 5, 2017 19:24

jaiminpanchal27 approved these changes Sep 5, 2017

View reviewed changes

matthewlane force-pushed the feature/video-stream-context branch 3 times, most recently from 0a024f5 to a2854c8 Compare September 8, 2017 19:42

mkendall07 approved these changes Sep 13, 2017

View reviewed changes

matthewlane added 12 commits September 13, 2017 14:28

Add support for video stream context

9bc0faa

Define adapter as supporting video

7b5d788

Use mediaTypes param to specify context

2bff874

Use utils.deepAccess

ac8fea9

Check for outstream bids

c581d4a

Add JSDoc and validation

81297c1

Rename functions and add unit test

3633165

Update property name

140675a

Update stubs to new sinon stub syntax

c9c366e

Only check context when mediaTypes.video was defined

97cd71f

Retain video-outstream compatibility

ccfc7ee

Revert to Sinon 1 syntax

0c8c9f1

matthewlane force-pushed the feature/video-stream-context branch from a2854c8 to 0c8c9f1 Compare September 13, 2017 22:14

Server and bid response ad type for any stream type is always 'video'

94bd7f2

matthewlane removed the in progress label Sep 13, 2017

mkendall07 requested changes Sep 15, 2017

View reviewed changes

Update to address code review

673dfef

mkendall07 merged commit fcf8d18 into master Sep 15, 2017

mkendall07 deleted the feature/video-stream-context branch September 15, 2017 19:56

matthewlane mentioned this pull request Nov 3, 2017

AdKernelAdn adapter #1747

Merged

9 tasks

matthewlane mentioned this pull request Nov 22, 2017

sekindoUM for prebid1.0 #1777

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for video stream context #1483

Add support for video stream context #1483

matthewlane commented Aug 14, 2017 •

edited

Loading

jaiminpanchal27 left a comment

jaiminpanchal27 Sep 5, 2017

jaiminpanchal27 Sep 5, 2017

jaiminpanchal27 Sep 5, 2017

matthewlane Sep 5, 2017

jaiminpanchal27 left a comment

mkendall07 left a comment

mkendall07 Sep 15, 2017

mkendall07 Sep 15, 2017

Add support for video stream context #1483

Add support for video stream context #1483

Conversation

matthewlane commented Aug 14, 2017 • edited Loading

Type of change

Description of change

Example AdUnits with Video Context

Other informatin

jaiminpanchal27 left a comment

Choose a reason for hiding this comment

jaiminpanchal27 Sep 5, 2017

Choose a reason for hiding this comment

jaiminpanchal27 Sep 5, 2017

Choose a reason for hiding this comment

jaiminpanchal27 Sep 5, 2017

Choose a reason for hiding this comment

matthewlane Sep 5, 2017

Choose a reason for hiding this comment

jaiminpanchal27 left a comment

Choose a reason for hiding this comment

mkendall07 left a comment

Choose a reason for hiding this comment

mkendall07 Sep 15, 2017

Choose a reason for hiding this comment

mkendall07 Sep 15, 2017

Choose a reason for hiding this comment

matthewlane commented Aug 14, 2017 •

edited

Loading