Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Triplelift: Add Instream support #5472

Merged
merged 17 commits into from
Aug 12, 2020
Merged

Triplelift: Add Instream support #5472

merged 17 commits into from
Aug 12, 2020

Conversation

sdao-tl
Copy link
Contributor

@sdao-tl sdao-tl commented Jul 8, 2020

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

  • Add support for instream video to TripleLift adapter

  • official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

  • Co-Author: Brandon Ling
  • TL ticket: TL-16738

Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

@sdao-tl,

Thanks for the PR. Can you please make the following changes:

  • Update the tripleliftBidAdapter.md file with an example of Instream video ad unit. We would require this so we can test whether we're getting a bid back or not when we send the request to your endpoint.

  • Please update the docs at https://github.com/prebid/prebid.github.io/ to document support for the video, instream media type.

@stale
Copy link

stale bot commented Jul 27, 2020

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 Jul 27, 2020
@stale stale bot removed the stale label Jul 27, 2020
@sdao-tl
Copy link
Contributor Author

sdao-tl commented Jul 27, 2020

@Fawke ,

  1. tripleliftAdapter.md changes done
  2. PR for docs update can be found here

@Fawke
Copy link
Contributor

Fawke commented Jul 28, 2020

@sdao-tl,

Thanks for providing an example adUnit in the md file. I'm trying to test the sample instream adUnit that you shared in a test page and getting {status: no_bid} as a response from your endpoint. Can you provide the correct test params so we can test it?

Also, you would need to make another change. From Prebid v4.0 onwards, publishers configuring prebid for video should now be able to pass video params in mediaTypes.video object. Read the blogpost for more details.

So, a publisher can configure the adunit like this:

{
    code: 'instream-div-1',
    mediaTypes: {
        video: {
            playerSize: [640, 480],
            context: 'instream',
            mimes: ['video/mp4'],
            w: 640,
            h: 480,
        }
    },
    bids: [
    {
        bidder: 'triplelift',
        params: {
            inventoryCode: 'instream_test'
        }
    }]
}

You can read the video params from either location, mediaTypes.video or bidder.params but give more precedence to mediaTypes.video if both are defined.

@sdao-tl
Copy link
Contributor Author

sdao-tl commented Aug 8, 2020

@Fawke
Thanks for the context, support for mediaTypes.video added.

RE: testing, unfortunately, we do not yet have a way to always return an instream ad so fill rate would depend on live auction. Would a cache bid work?

@Fawke
Copy link
Contributor

Fawke commented Aug 10, 2020

@sdao-tl,

I'm fine with your first change: giving more precedence to mediaTypes.video.

We generally check few things for an instream bid response. If you can verify that the response passes these checks, we can get this merged.

  • Bid response must have Content-Encoding set to gzip
  • You are returning at least one of there params, bid.videoCacheKey, bid.vastXml, or bid.vastUrl.

It's also good to have some meta information about the bid. For example, advertiserId. Read the blogpost's meta taxonomy section for more details. It's not mandatory to do this part though.

@Fawke Fawke removed the needs docs label Aug 10, 2020
@sdao-tl
Copy link
Contributor Author

sdao-tl commented Aug 11, 2020

@Fawke ,

I have attached my chrome network logs which contain the request/response for the instream ad. In it, you can confirm our gzip encoding.

Regarding the params requirement: for instream responses we send the vast xml in the ad field and this gets mapped to the required param vastXml in our adapter (L245-247). Does this meet the requirement?

triplelift-instream-example.har.zip

@Fawke Fawke added LGTM and removed needs update labels Aug 12, 2020
@Fawke Fawke merged commit 4d2b401 into prebid:master Aug 12, 2020
@patmmccann patmmccann mentioned this pull request Jul 30, 2020
10 tasks
BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
* initial commit, instream poc done

* push in poc changes

* push in poc changes

* restore instream.html

* push in poc changes

* restore instream.html

* restore instream.html v2

* adding instream unit tests v1

* catch up to bidfloor changes

* unit tests finalized!

* update adapter md

* add support for mediaTypes.video

Co-authored-by: Sy Dao <iam.sydao@gmail.com>
BrightMountainMediaInc added a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
tagid: bidRequest.params.inventoryCode,
floor: _getFloor(bidRequest)
};
if (bidRequest.mediaTypes.video) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this appears to be bad logic; multiformat outstream / banner / native units will be considered video units

Copy link
Collaborator

Choose a reason for hiding this comment

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

the effect is you don't bid on them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Pat,

Thanks for surfacing! We will review internally and update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @patmmccann ,

I've opened PR 5872 to resolve this -- thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants