Skip to content

Feature/smart video#4367

Merged
jsnellbaker merged 11 commits intoprebid:masterfrom
smartadserver:feature/smart-video
Nov 20, 2019
Merged

Feature/smart video#4367
jsnellbaker merged 11 commits intoprebid:masterfrom
smartadserver:feature/smart-video

Conversation

@tadam75
Copy link
Contributor

@tadam75 tadam75 commented Oct 23, 2019

Type of change

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

Description of change

In this pull request, we add the support of video media type (instream & outstream) for the Smart AdServer bid adapter.

@jsnellbaker jsnellbaker self-requested a review October 23, 2019 16:09
@jsnellbaker jsnellbaker self-assigned this Oct 23, 2019
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @tadam75

While the instream changes look ok (I was able to see the test video play successfully), there are some issues with how the outstream bids are handled.

The following page can provide some additional information on what outstream ads require if needed:
http://prebid.org/dev-docs/show-outstream-video-ads.html#renderers

} else if (videoMediaType && videoMediaType.context === 'instream') {
// Specific attributes for instream.
var playerSize = videoMediaType.playerSize[0];
payload.isVideo = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This variable is causing the returned outstream bid from your adserver to be setup as a banner creative (see line 126 below). The outstream bid should be treated as a video mediaType and ideally have an associated renderer.

@stale
Copy link

stale bot commented Nov 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 7, 2019
@tadam75
Copy link
Contributor Author

tadam75 commented Nov 12, 2019

Hi @jsnellbaker

When we receive a request for outstream, everything is embedded in the ad-markup that we respond (the player and the content) and it can't be separated. That's why it's handled just like a banner creative.

Do you see a way to fit in your outstream video integration ?
Thanks in advance.

@stale stale bot removed the stale label Nov 12, 2019
@tadam75 tadam75 requested a review from jsnellbaker November 18, 2019 09:22
@jsnellbaker
Copy link
Collaborator

Hi @tadam75

At the moment, I'm not sure there is a way to make this type of approach work (while still classifying the bid response object as a proper outstream video bid).

I've reached out to several other members of the Prebid team to get their take on this approach and any feedback they have on the direction being attempted here. I'll try to follow-up once there's something definitive.

In the meantime, if you want to add partial instream video support for your adapter (ie drop the code parts that support outstream) - we could accept that and get it into master. If so - we could open an issue on the outstream question, track that conversation separately, and then (depending on the feedback) you could open a separate PR later to add the outstream support back to the adapter.

If you want to go this route, please let me know and make the updates to this PR.

Thanks.

@tadam75
Copy link
Contributor Author

tadam75 commented Nov 20, 2019

@jsnellbaker ,

Thanks for your detailed response.

As you suggested, we removed the outstream specific code to have instream video support.

Do you want me to open the issue for our outstream case ?

@jsnellbaker
Copy link
Collaborator

@tadam75 If you can, that would be great. I'll look at the updates here and follow-up.

@jsnellbaker
Copy link
Collaborator

Hi @tadam75
The updates LGTM. I'll go ahead an merge this PR.

@tadam75 tadam75 deleted the feature/smart-video branch April 3, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments