ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

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

Closed shawnbuso closed 6 years ago

shawnbuso commented 7 years ago

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 commented 7 years ago

/to @cramforce

cramforce commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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!

aghassemi commented 7 years ago

@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 commented 7 years ago

Hey @shawnbuso any updates on how this coming along?

shawnbuso commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

shawnbuso commented 7 years ago

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 commented 7 years ago

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

aghassemi commented 7 years ago

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

jasti commented 7 years ago

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

shawnbuso commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

aghassemi commented 7 years ago

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)

ghost commented 7 years ago

+1 I really want IMA on AMP 👍

jasti commented 7 years ago

Hey @shawnbuso - was there an update on this?

shawnbuso commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Is there a link where we could take a look?

On Fri, Jan 27, 2017 at 12:18 PM, Shawn Busolits notifications@github.com wrote:

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: [image: screen shot 2017-01-27 at 3 17 13 pm] https://cloud.githubusercontent.com/assets/1238504/22386142/bb670d1e-e4a3-11e6-8f21-e31d7dcccade.png 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: [image: screen shot 2017-01-27 at 3 15 58 pm] https://cloud.githubusercontent.com/assets/1238504/22386123/9ce791d8-e4a3-11e6-8683-cee5ea519152.png Any idea what's happening that's causing the page to more or less double the size of my video player?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ampproject/amphtml/issues/5233#issuecomment-275763894, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFeT9kqjaFUcBFXoYxr6JfZbYm5e6koks5rWlD6gaJpZM4KG2So .

aghassemi commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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)

jasti commented 7 years ago

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

shawnbuso commented 7 years ago

Thanks! I've got just a few things:

jasti commented 7 years ago

@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 commented 7 years ago
  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 commented 7 years ago

We can help with 3rd party sample.

On Feb 10, 2017, at 2:59 PM, Shawn Busolits notifications@github.com wrote:

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 tag

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.

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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

jasti commented 7 years ago

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

aghassemi commented 7 years ago

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

shawnbuso commented 7 years ago

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.

aghassemi commented 6 years ago

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

GitHubgopinath commented 3 years ago

Hello everyone, Please give an example code for the Responsive video ad in Google ads

akshatrana02 commented 2 months ago

Hi everyone, I have an query regarding amp-ima-video element. As it not playing VAST tag for ads.