Intent-to-Implement: Ad-enabled video player using the IMA SDK #5233

Closed
shawnbuso opened this Issue Sep 26, 2016 · 35 comments

Comments

@shawnbuso
Contributor

shawnbuso commented Sep 26, 2016

We want to create a new extension, <amp-ima-video>, which takes a content URL and a video ad tag and provides an ad-enabled video player with your content and ads. This will use Google's IMA SDK. For a demo of the IMA SDK, see our sample. Sample tag:

<amp-ima-video
    width="640px"
    height="360px"
    src="VIDEO_URL"
    ad-tag-url="AD_TAG_URL">
</amp-ima-video>

This will render on the page a video player, which will play the provided content and ads. We're looking for implementation advice. Specifically, I'm having trouble getting the ima3.js library loaded and available to the extension without using an iframe for the extension itself, which is not necessary for our implementation. Also, if possible, our extension could extend the existing amp-video element (instead of BaseElement).

@jridgewell

This comment has been minimized.

Show comment
Hide comment
Member

jridgewell commented Sep 26, 2016

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Sep 26, 2016

Member

Not using an iframe will only be possible if https://developers.google.com/interactive-media-ads/docs/sdks/html5/ is open source with a compatible license and can be fully included in AMP. That is likely not such a great idea.

The next option is to host your own iframe, or use an 3p-frame based implementation. amp-twitter and amp-facebook use that mechanism.

@aghassemi Can guide you through implementing the new video interface which is now required for video players.

CC @jasti

Member

cramforce commented Sep 26, 2016

Not using an iframe will only be possible if https://developers.google.com/interactive-media-ads/docs/sdks/html5/ is open source with a compatible license and can be fully included in AMP. That is likely not such a great idea.

The next option is to host your own iframe, or use an 3p-frame based implementation. amp-twitter and amp-facebook use that mechanism.

@aghassemi Can guide you through implementing the new video interface which is now required for video players.

CC @jasti

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Sep 27, 2016

Member

Had a quick chat with @jasti, I will take a look at some IMA samples and come up with some approaches we can discuss.

Member

aghassemi commented Sep 27, 2016

Had a quick chat with @jasti, I will take a look at some IMA samples and come up with some approaches we can discuss.

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Sep 27, 2016

Contributor

Thanks for the info. The SDK itself is not open source, but the implementation can be (we already have an open source implementation for video.js). I look forward to your input!

Contributor

shawnbuso commented Sep 27, 2016

Thanks for the info. The SDK itself is not open source, but the implementation can be (we already have an open source implementation for video.js). I look forward to your input!

@ericlindley-g ericlindley-g added this to the Pending 3P Implementation milestone Sep 28, 2016

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Oct 31, 2016

Member

@shawnbuso FYI that <amp-youtube> now supports the video-interface and we have a compliance test suite in place. Please see https://github.com/ampproject/amphtml/blob/master/extensions/amp-youtube/0.1/amp-youtube.js and https://github.com/ampproject/amphtml/blob/master/test/integration/test-video-players.js or #5765 for details.

Member

aghassemi commented Oct 31, 2016

@shawnbuso FYI that <amp-youtube> now supports the video-interface and we have a compliance test suite in place. Please see https://github.com/ampproject/amphtml/blob/master/extensions/amp-youtube/0.1/amp-youtube.js and https://github.com/ampproject/amphtml/blob/master/test/integration/test-video-players.js or #5765 for details.

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Dec 2, 2016

Collaborator

Hey @shawnbuso any updates on how this coming along?

Collaborator

jasti commented Dec 2, 2016

Hey @shawnbuso any updates on how this coming along?

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Dec 19, 2016

Contributor

I'm making pretty good progress here but I'm hung up on implementing the video interface. YouTube has all of their logic in their AmpYoutube object, which makes it pretty easy. All of my logic, however, is in the functions in ads/google/imaVideo.js, which isn't extending AMP.BaseElement. Are there any examples of getting the code in my object at extensions/amp/ima-video/0.1/amp-ima-video.js to interact with my code in ads/google/imaVideo.js? I'm still unsure how or if those different code segments can interact with each other.

Contributor

shawnbuso commented Dec 19, 2016

I'm making pretty good progress here but I'm hung up on implementing the video interface. YouTube has all of their logic in their AmpYoutube object, which makes it pretty easy. All of my logic, however, is in the functions in ads/google/imaVideo.js, which isn't extending AMP.BaseElement. Are there any examples of getting the code in my object at extensions/amp/ima-video/0.1/amp-ima-video.js to interact with my code in ads/google/imaVideo.js? I'm still unsure how or if those different code segments can interact with each other.

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Dec 19, 2016

Member

@shawnbuso so your code runs inside the iframe, right? Your code in your AMP component would look similar to youtube, but you have to pass messages via postMessage from an do the iframe.

Member

cramforce commented Dec 19, 2016

@shawnbuso so your code runs inside the iframe, right? Your code in your AMP component would look similar to youtube, but you have to pass messages via postMessage from an do the iframe.

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Dec 20, 2016

Contributor

Ok thanks, I wasn't sure if AMP had any kind of messaging channel set up already. I'll give that a shot!

Contributor

shawnbuso commented Dec 20, 2016

Ok thanks, I wasn't sure if AMP had any kind of messaging channel set up already. I'll give that a shot!

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Jan 4, 2017

Contributor

OK looking for advice on one more thing - it looks like the the video interface has really good support for autoplay, but unfortunately the IMA SDK doesn't :/ We have a method that initializes a video player which must be called as the result of a user action. What's the best way for me to handle auto-play in this situation? Should I disable ads or drop support for auto-play?

Contributor

shawnbuso commented Jan 4, 2017

OK looking for advice on one more thing - it looks like the the video interface has really good support for autoplay, but unfortunately the IMA SDK doesn't :/ We have a method that initializes a video player which must be called as the result of a user action. What's the best way for me to handle auto-play in this situation? Should I disable ads or drop support for auto-play?

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Jan 4, 2017

Member

@shawnbuso I am starting a document regarding autoplay and ads, will share it soon.

Member

aghassemi commented Jan 4, 2017

@shawnbuso I am starting a document regarding autoplay and ads, will share it soon.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Jan 6, 2017

Member

@shawnbuso Is is possible to turnoff ads only when autoplay is set for now until we have a plan autoplay and ads?

Member

aghassemi commented Jan 6, 2017

@shawnbuso Is is possible to turnoff ads only when autoplay is set for now until we have a plan autoplay and ads?

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Jan 6, 2017

Collaborator

@shawnbuso why does the IMA SDK not have autoplay support?

Collaborator

jasti commented Jan 6, 2017

@shawnbuso why does the IMA SDK not have autoplay support?

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Jan 10, 2017

Contributor

I believe it's that we never provided an option to initialize the ad player in muted mode, so I need to work with our eng team to get that added. I'm running some tests now though to see if I can put together a fix myself.

On another note, my manual test page starting failing today and I have no idea why, so I'm hoping someone can help me out. When I run gulp and navigate to http://localhost:8000/examples/ima-video.amp.html the console shows that I get a 404 trying to load https://cdn.ampproject.org/v0/amp-ima-video-0.1.js. Any idea what's going wrong here? I wouldn't expect any of my files to be on amppproject.org since the plugin isn't published, but AFAIK I have always had a script tag in my example loading this JS file - I assumed gulp recognized that it was a dev extension and proxied the connection to serve some local copy, but I could be wrong.

Contributor

shawnbuso commented Jan 10, 2017

I believe it's that we never provided an option to initialize the ad player in muted mode, so I need to work with our eng team to get that added. I'm running some tests now though to see if I can put together a fix myself.

On another note, my manual test page starting failing today and I have no idea why, so I'm hoping someone can help me out. When I run gulp and navigate to http://localhost:8000/examples/ima-video.amp.html the console shows that I get a 404 trying to load https://cdn.ampproject.org/v0/amp-ima-video-0.1.js. Any idea what's going wrong here? I wouldn't expect any of my files to be on amppproject.org since the plugin isn't published, but AFAIK I have always had a script tag in my example loading this JS file - I assumed gulp recognized that it was a dev extension and proxied the connection to serve some local copy, but I could be wrong.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Jan 10, 2017

Member

@shawnbuso For the manual test pages issue, please add <script async custom-element="amp-video" src="../../dist/v0/amp-video-0.1.max.js"></script> to the page similar to how this PR #6868 did for other manual test pages

Member

aghassemi commented Jan 10, 2017

@shawnbuso For the manual test pages issue, please add <script async custom-element="amp-video" src="../../dist/v0/amp-video-0.1.max.js"></script> to the page similar to how this PR #6868 did for other manual test pages

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Jan 10, 2017

Contributor

Makes sense, thanks. Not sure how I managed to accidentally change that.

Contributor

shawnbuso commented Jan 10, 2017

Makes sense, thanks. Not sure how I managed to accidentally change that.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Jan 10, 2017

Member

Not your fault :) we made a change that was not backward compatible for manual test pages ( due to the unique way manualtest pages include AMP scripts)

Member

aghassemi commented Jan 10, 2017

Not your fault :) we made a change that was not backward compatible for manual test pages ( due to the unique way manualtest pages include AMP scripts)

@JHGitty

This comment has been minimized.

Show comment
Hide comment
@JHGitty

JHGitty Jan 25, 2017

+1 I really want IMA on AMP 👍

JHGitty commented Jan 25, 2017

+1 I really want IMA on AMP 👍

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Jan 25, 2017

Collaborator

Hey @shawnbuso - was there an update on this?

Collaborator

jasti commented Jan 25, 2017

Hey @shawnbuso - was there an update on this?

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Jan 25, 2017

Contributor

No update yet, but I'm still working on it. With my other responsibilities this is only getting 10-20% of my time. Currently I'm working on fixing autoplay (which I think might be an IMA SDK issue), fully testing on mobile, and writing unit tests. Once those 3 things are done I should be ready to send out a PR. Our goal is to have this submitted by the end of this quarter.

Contributor

shawnbuso commented Jan 25, 2017

No update yet, but I'm still working on it. With my other responsibilities this is only getting 10-20% of my time. Currently I'm working on fixing autoplay (which I think might be an IMA SDK issue), fully testing on mobile, and writing unit tests. Once those 3 things are done I should be ready to send out a PR. Our goal is to have this submitted by the end of this quarter.

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Jan 27, 2017

Contributor

OK autoplay is not an IMA SDK issue - I can get that working just fine in an IMA implementation outside of AMP. I think the issue now is that part of my video is outside of the display port... but it shouldn't be. My element is set to 640x360, which should easily fit inside of the viewport for mobile safari. For some reason, though, when I load the page my video is huge and the entire right half is outside the viewport:
screen shot 2017-01-27 at 3 17 13 pm
If I inspect using the safari inspector it insists that the video is only 640x360, but when I load a page that just has a 640x360 video player (no AMP) it fits on the page fine:
screen shot 2017-01-27 at 3 15 58 pm
Any idea what's happening that's causing the page to more or less double the size of my video player? You can see my current test version at http://amp-ima-video.herokuapp.com/examples/ima-video.amp.html.

Contributor

shawnbuso commented Jan 27, 2017

OK autoplay is not an IMA SDK issue - I can get that working just fine in an IMA implementation outside of AMP. I think the issue now is that part of my video is outside of the display port... but it shouldn't be. My element is set to 640x360, which should easily fit inside of the viewport for mobile safari. For some reason, though, when I load the page my video is huge and the entire right half is outside the viewport:
screen shot 2017-01-27 at 3 17 13 pm
If I inspect using the safari inspector it insists that the video is only 640x360, but when I load a page that just has a 640x360 video player (no AMP) it fits on the page fine:
screen shot 2017-01-27 at 3 15 58 pm
Any idea what's happening that's causing the page to more or less double the size of my video player? You can see my current test version at http://amp-ima-video.herokuapp.com/examples/ima-video.amp.html.

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Jan 27, 2017

Member
Member

cramforce commented Jan 27, 2017

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Jan 27, 2017

Member

@shawnbuso I think you just need layout="responsive" on the amp-ima-video. currently width and height are set to width=640 height=360 without layout="responsive" they will be interpreted as fixed. With layout="responsive" only the aspect ratio will be used and it will be resized approprietly to fit the width.

Member

aghassemi commented Jan 27, 2017

@shawnbuso I think you just need layout="responsive" on the amp-ima-video. currently width and height are set to width=640 height=360 without layout="responsive" they will be interpreted as fixed. With layout="responsive" only the aspect ratio will be used and it will be resized approprietly to fit the width.

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Jan 30, 2017

Contributor

@cramforce You can see the current version at http://amp-ima-video.herokuapp.com/examples/ima-video.amp.html

@aghassemi Right now my implementation is reliant on the pixel width and height provided in the data from the tag, so I don't think I can support responsive layout. The IMA SDK also requires that you initialize it with the pixel width and height of your ad container, so if the ad container size changes after initialization it will cause problems. Still, I don't think I should need to support responsive to fix this particular issue - I'm telling it to create a DOM element that is x by y, and it says it has done so, but the element renders on the screen as being much much bigger than x by y.

Contributor

shawnbuso commented Jan 30, 2017

@cramforce You can see the current version at http://amp-ima-video.herokuapp.com/examples/ima-video.amp.html

@aghassemi Right now my implementation is reliant on the pixel width and height provided in the data from the tag, so I don't think I can support responsive layout. The IMA SDK also requires that you initialize it with the pixel width and height of your ad container, so if the ad container size changes after initialization it will cause problems. Still, I don't think I should need to support responsive to fix this particular issue - I'm telling it to create a DOM element that is x by y, and it says it has done so, but the element renders on the screen as being much much bigger than x by y.

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Jan 30, 2017

Contributor

@aghassemi scratch what I said - I can support responsive layout as long as I can grab the actual width and height of my iframe from global.innerWidth and global.innerHeight in my getIma method.

Contributor

shawnbuso commented Jan 30, 2017

@aghassemi scratch what I said - I can support responsive layout as long as I can grab the actual width and height of my iframe from global.innerWidth and global.innerHeight in my getIma method.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Jan 30, 2017

Member

@shawnbuso Good to hear that responsive can be supported. We also need to handle size change ( e.g. device rotation ).
If you need width/height inside the component code, onLayoutMeasure callback is what you want. You can get the calculated width/height from this.getLayoutBox(); and onLayoutMeasure is called initially and every time the size changes.

If you are inside an iframe, you can listen to the resize event of the iframe window in addition to the initial init. (or another option is still relying on component's onLayoutMeasure and send the new width/height through postMessage to the iframe)

Member

aghassemi commented Jan 30, 2017

@shawnbuso Good to hear that responsive can be supported. We also need to handle size change ( e.g. device rotation ).
If you need width/height inside the component code, onLayoutMeasure callback is what you want. You can get the calculated width/height from this.getLayoutBox(); and onLayoutMeasure is called initially and every time the size changes.

If you are inside an iframe, you can listen to the resize event of the iframe window in addition to the initial init. (or another option is still relying on component's onLayoutMeasure and send the new width/height through postMessage to the iframe)

@cramforce cramforce removed their assignment Feb 5, 2017

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Feb 10, 2017

Collaborator

@shawnbuso http://amp-ima-video.herokuapp.com/examples/ima-video.amp.html looks awesome!
Were there other blockers before launching this?

Collaborator

jasti commented Feb 10, 2017

@shawnbuso http://amp-ima-video.herokuapp.com/examples/ima-video.amp.html looks awesome!
Were there other blockers before launching this?

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Feb 10, 2017

Contributor

Thanks! I've got just a few things:

  • The UI is pretty ugly. Right now I'm using unicode characters for play, pause, and fullscreen, but I think it will look better with actual image icons. Do you guys have something you use for AMP in general or should I create my own?

  • Autoplay still doesn't work on Android. This is an IMA issue and I've proposed a fix to the eng team, so I'm waiting to see if they can implement that or not.

  • I still need to write some automated tests - I have it in the video test framework and those are passing, but I don't have any real tests of my own outside of that.

Contributor

shawnbuso commented Feb 10, 2017

Thanks! I've got just a few things:

  • The UI is pretty ugly. Right now I'm using unicode characters for play, pause, and fullscreen, but I think it will look better with actual image icons. Do you guys have something you use for AMP in general or should I create my own?

  • Autoplay still doesn't work on Android. This is an IMA issue and I've proposed a fix to the eng team, so I'm waiting to see if they can implement that or not.

  • I still need to write some automated tests - I have it in the video test framework and those are passing, but I don't have any real tests of my own outside of that.

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Feb 10, 2017

Collaborator

@aghassemi is on vaca

  1. any chance we can use the controls from amp-video?
  2. Shouldn't be a blocker for our launch
  3. Some samples delivering ads from DFP, AdX and 3rd party tags would also be helpful.

@jpettitt FYI, on your request for a default player with ability to monetize.

Collaborator

jasti commented Feb 10, 2017

@aghassemi is on vaca

  1. any chance we can use the controls from amp-video?
  2. Shouldn't be a blocker for our launch
  3. Some samples delivering ads from DFP, AdX and 3rd party tags would also be helpful.

@jpettitt FYI, on your request for a default player with ability to monetize.

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Feb 10, 2017

Contributor
  1. I'd love to but the IMA SDK isn't compatible with native video controls (it uses overlays that block them) and I'm not sure how to extract the assets from the video player since it's all just one <video> tag

  2. I can try to create a workaround for it, but right now if you use autoplay on Android you get the first frame of the ad and it just freezes there, which is a pretty bad user experience. I can either skip ads altogether on Android with autoplay (which would be pretty hacky with how the plugin is currently written) or just ignore autoplay on Android and make the user click to start the video.

  3. I can definitely get DFP, AdX, and AdSense samples. Will have to see about getting test 3p tags. All should work out of the box as long as they return a valid VAST response.

Contributor

shawnbuso commented Feb 10, 2017

  1. I'd love to but the IMA SDK isn't compatible with native video controls (it uses overlays that block them) and I'm not sure how to extract the assets from the video player since it's all just one <video> tag

  2. I can try to create a workaround for it, but right now if you use autoplay on Android you get the first frame of the ad and it just freezes there, which is a pretty bad user experience. I can either skip ads altogether on Android with autoplay (which would be pretty hacky with how the plugin is currently written) or just ignore autoplay on Android and make the user click to start the video.

  3. I can definitely get DFP, AdX, and AdSense samples. Will have to see about getting test 3p tags. All should work out of the box as long as they return a valid VAST response.

@dknecht

This comment has been minimized.

Show comment
Hide comment
@dknecht

dknecht Feb 10, 2017

Contributor
Contributor

dknecht commented Feb 10, 2017

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Feb 18, 2017

Collaborator

@shawnbuso My customary weekly ping on this issue :). @aghassemi do you have thoughts on 1, above?

Collaborator

jasti commented Feb 18, 2017

@shawnbuso My customary weekly ping on this issue :). @aghassemi do you have thoughts on 1, above?

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Feb 18, 2017

Member

amp-video uses native video controls and does not customize them in anyway.

Member

aghassemi commented Feb 18, 2017

amp-video uses native video controls and does not customize them in anyway.

@shawnbuso

This comment has been minimized.

Show comment
Hide comment
@shawnbuso

shawnbuso Feb 21, 2017

Contributor

Yea I think our biggest issue with native controls revolves around fullscreen - we can't support native fullscreen, and as far as I can tell there's no elegant way to disable that and implement my own while still using the native controls.

Contributor

shawnbuso commented Feb 21, 2017

Yea I think our biggest issue with native controls revolves around fullscreen - we can't support native fullscreen, and as far as I can tell there's no elegant way to disable that and implement my own while still using the native controls.

@jasti jasti removed this from Current - non-Core Ads in AMP Advertising Feb 22, 2017

@aghassemi aghassemi moved this from Prioritized Backlog to Backlog in Video Mar 14, 2017

@jasti jasti modified the milestones: Active Non-Sprint Tracked, Pending 3P Implementation Mar 23, 2017

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Oct 18, 2017

Member

Closing since amp-ima-video it is now in doc-level experiment

Member

aghassemi commented Oct 18, 2017

Closing since amp-ima-video it is now in doc-level experiment

@aghassemi aghassemi closed this Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment