search

ampproject / amphtml

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

I2I: Cloudinary amp extension #21500

Closed nitzanj closed 3 years ago

nitzanj commented 5 years ago

Background:

Cloudinary is a media delivery, transformation and optimization platform. Cloudinary allows developers to automatically optimize media dimensions, format and compression to achieve the best balance between quality and page load times. Cloudinary also provides a very wide selection of transformations and effects, applied dynamically to the resource upon delivery (server-side).

Summary

We intend to implement a Cloudinary amp extension to display media delivered through Cloudinary service. The extension will allow developers to declare transformations and effects using attributes, and will set the underlying src attribute automatically. The extension also injects (if requested) the run-time element dimensions into the url to deliver dynamic on-the-fly responsive media.

Motivation

Many Cloudinary customers are providing (or planning to provide) AMP pages, and they struggle with integrating Cloudinary successfully. Using a custom Cloudinary extension, Cloudinary's abilities (media transformations, effects, optimizations, responsive) are readily available and easily integrated into AMP pages.

Without a dedicated extension, developers will need to generate the complex delivery urls manually, or in other parts of their pipeline, where it's less fitting. Some features will be completely out of scope (automatic responsive media).

Usage example:

A responsive layout with smart cropping and the sepia effect applied:

<amp-cld-img
    width="400"
    height="200"
    layout="responsive"
    cloud-name="demo"
    data-public-id="sample"
    effect="sepia"
    crop="fill"
    format="auto"
    quality="auto"
    gravity="auto">
</amp-cld-img>

In this example the image format, compression and dimensions are automatically determined for each user, and personally optimised.

/cc @ampproject/wg-approvers /cc @ampproject/wg-ui-and-a11y /cc @cathyxz

cathyxz commented 5 years ago

Hi @nitzanj, thanks for filing this!

A couple of clarification questions:

Without a dedicated extension, developers will need to generate the complex delivery urls manually, or in other parts of their pipeline, where it's less fitting. Some features will be completely out of scope (automatic responsive media).

The extension also injects (if requested) the run-time element dimensions into the url to deliver dynamic on-the-fly responsive media.

compression and dimensions are automatically determined for each user, and personally optimised

A few implementation considerations:

I think my questions boil down to: What does your proposed extension offer that the current <amp-img> + css cannot do, whether that should involve creating a new extension, or is this largely a reflection of missing features or API pain points in <amp-img>. If there's a lot of the latter, we would love your feedback. =)

Thank you!

nitzanj commented 5 years ago

Hi @cathyxz,

Thank you for taking the time to review this request. I hope I answered all the comments/questions raised, I'll be happy to answer any followup questions you may have.

What are the downsides to solving url generation by providing a server-side API?

It's possible to generate the URL in the server. The downside is that the definition of the image dimension and manipulation is then handled by the server developer rather than the page author. Cloudinary provides much stronger on-the-fly creative capabilities without requiring server integration. This avoids much of the necessity for advanced image editing in external tools like photoshop. A few examples of these capabilities include dynamic smart cropping, scaling, padding, adding overlays, creative effects, conditional transformations (specifying image-aware directives) and much more, all available to the page author directly within the page code.

What are your automatic responsiveness heuristics? For context, I'm wondering:

  • Are they compatible with the resource lifecycle in AMP, implementation-wise?
  • Are these potentially missing features that we should be looking to add to <amp-img> as opposed to writing a new extension?

Cloudinary enables developers to specify image dimensions directly in the URL. When that URL is requested, the image is generated and cached for future requests. With Cloudinary and similar solutions, the developer doesn't need to prepare the corresponding image in advance. The images are generated using the requested dimensions on the fly. Using the proposed Cloudinary extension, the runtime viewport dimensions will automatically be used as the width/height inside the URL. This means that each user loading the page will get images in the exact dimensions suitable for their actual viewport size. Since the URL - and the corresponding image - is generated at runtime, it is not possible or practical to prefetch the image in the AMP preconnect callback when the viewport dimensions are not yet known. In theory, that could potentially be solved using URL templating. However, a full solution - supporting various runtime values as well as various image-processing services - would probably be much too complex and impractical to implement directly in amp-img

What are other missing features that are currently out of scope?

Many of Cloudinary’s more advanced and significant features are CDN-based, such as:

The features above result in a significant reduction in page load time, and optimal rendering of the image on the user agent. Those features cannot work reliably when going through the pre-connect phase and the amp-cache. We've also observed many images getting double-optimized and actually ending up with a larger filesize.

The extension also injects (if requested) the run-time element dimensions into the url to deliver dynamic on-the-fly responsive media.

Could you elaborate on what that means? Is this doing something that srcset and sizes is cannot achieve?

In order to support integration with page-generation frameworks, this extension allows modifying pre-constructed urls - replacing placeholders with the runtime width or height. Other than that, this is identical to the solution outlined above. Some of the behaviour can be replicated with srcset and sizes. However, our solution allows for far greater granulation and control with far less verbosity (it’s possible to have a different src for each 10 pixels of viewport size with just one html attribute).

compression and dimensions are automatically determined for each user, and personally optimised

  • What does "personally optimised" mean here, and what information does the extension need to send back to your server to do that personal optimisation?

This is explained above under the advanced features.

A few implementation considerations:

  • What attributes of <amp-cld-img> do you foresee will need to be bindable?
  • What events or actions might you need / want to expose?

I think my questions boil down to: What does your proposed extension offer that the current <amp-img> + css cannot do, whether that should involve creating a new extension, or is this largely a reflection of missing features or API pain points in <amp-img>. If there's a lot of the latter, we would love your feedback. =)

As explained above, the proposed solution is probably not something that amp-img can (or should) do. Some of the features may be replicated in amp-img by using customized cache behaviour and url-templating, but it won’t cover all the features and will add unnecessary complexity to amp-img.

Thanks!

cathyxz commented 5 years ago

Got it. Thank you very much @nitzanj for the explanation! This sounds great. And

dynamic smart cropping, scaling, padding, adding overlays, creative effects, conditional transformations (specifying image-aware directives) and much more, all available to the page author directly within the page code.

Wow.

all cloudinary assets are served using several CDNs so our users can benefit from better performance

Nice!

Two more things to possibly think about:

  1. Layouts For lazy loading purposes, most AMP layouts require specifying image dimensions / aspect ratios / height of all components prior to runtime. So even though you may determine the image size at runtime, your component size must be determined prior for most layouts.
  2. srcset and sizes delegate upgrading images on viewport size changes to the browser's native behavior. If you're not using that, then you'll likely have to handle that within the component.

Everything else sounds great. Building an AMP Extension has a lot of good documentation on how-tos / best practices in writing amp extensions (if this is your first extension, I suggest looking over the AMP Element callbacks and vsync / mutateElement sections). If you need help during the implementation process, feel free to ping me on slack (czhu) as well. Otherwise I look forward to reviewing your PR (please do tag me in your PR description for review so it doesn't slip through the cracks).

Thank you for your contribution! =)

cathyxz commented 5 years ago

Hi @nitzanj! I wanted to check in and give you a heads up on the following:

  1. If you need help with your implementation or have questions, feel free to ping myself (czhu) or the wg-ui-and-ally channel on slack.
  2. If at any point you feel like there are multiple options in your component design or that aspects of it may be controversial (I currently don't see any that are obviously so but this is a resource available to you) and would like to have a discussion with the broader AMP community, you are welcome to put this issue up in Design Review (just link to it as a comment under one of the open issues with the Design Review label ).
  3. Most of the AMP Team will be in Tokyo attending AMP Conf during the week following April 13. Please expect delays in responses and PR reviews during this time.

Again, feel free to reach out if you need help or have questions. =)

aghassemi commented 5 years ago

For the extension name, I suggest amp-cloudinary-image . We also need 3 approvals from @ampproject/wg-approvers to move forward. 1 LGTM from me.

jridgewell commented 5 years ago

LGTM, too. Note that these will not be pre-renderable images, because we won't be able to deliver them via cdn.ampproject.org (or any other AMP Cache) image cache. We actually could make them deliverable that way, if we make the final URLs more static (not dependent on the requesting browser, only the effects applied to the element).

Gregable commented 5 years ago

LGTM. This is one of those great cases for using images not from the AMP Cache. Justin's point is correct though: We want these to not be pre-renderable until we find a mechanism to load them via the AMP Cache directly. I think that's a reasonable tradeoff in this case. @ampproject/wg-caching as an FYI regarding the unique caching requirements.

nitzanj commented 5 years ago

Thanks for the input!

Indeed our initial implementation (see pr #22149) does not use the pre-connect callback at all, there's just no way to pre-render with the current mechanisms. But considering that all of Cloudinary's resources are always delivered through CDNs, we too believe that the trade-off is worth it. Also, the vast majority of Cloudinary's customers use our automatic optimizations before delivery, and as mentioned above, images that go through the amp-cache are double-optimized, and that's usually bad for both visual quality and file size...

cramforce commented 5 years ago

I'd like us to consider a view things:

sparhami commented 5 years ago

@nitzanj There are a few questions the wider AMP team may have that we should answer about this extension before launching this. I think a design review would be a great format to make sure we have everyone on board.

Take a look at the up coming design review dates here: https://github.com/ampproject/amphtml/labels/Type%3A%20Design%20Review and add a comment to one of them that works for you that you would like to discuss this i2i, with a link to this i2i. See https://github.com/ampproject/amphtml/blob/master/contributing/design-reviews.md for more information about design reviews.

sparhami commented 5 years ago

One concern that came up is that, because this does not use <amp-img>, images rendered with this extension will not work with <amp-lightbox-gallery>. Regardless of how we proceed, we should make sure that lightbox functionality works.

kristoferbaxter commented 5 years ago

I too share the concerns brought up by Malte and Sepand. This could make it more difficult to pre-render parts of a document for caches.

Is there a reason we need to express these features in client-side code instead of using a server based approach for declaration?

dynamic smart cropping, scaling, padding, adding overlays, creative effects, conditional transformations (specifying image-aware directives) and much more, all available to the page author directly within the page code.

These transformations could happen on the various srcset sizes in a server API driven design I believe. Is there something unique to being on the client that necessitates moving this complexity out of the server?

Smart per-user optimizations - Cloudinary delivers images to each individual user the optimal image format, balancing quality and file size. Automatic compression quality is also calculated on a per-image basis, depending on the format. This is based on user agent: OS, device, browser, versions.

If the implementation was entirely server driven, the generated URLs could still be serviced by the Cloudinary services allowing for the permutations as expected.

nitzanj commented 5 years ago

Following the design review, here are the main takeouts:

Alternative Benefits Problems
Url injection Optimal image dimensions - both height and width tailored to the viewport. Server-based smart content aware cropping. Does not support pre-loading/pre-caching. Client side processing required.
srcset (static or dynamic) pre-loading/pre-caching. Only viewport width is taken into account. Further vertical cropping/scaling is client based, not content aware. Client side processing may be required (if srcset is generated dynamically).
Client hints No client side processing, automatic smart width breakpoint mechanism applied. Only works where client hints are supported. Only viewport width is taken into account. Further vertical cropping/scaling is client based, not content aware.

We've chosen to go with the first approach. Client hints is not widely supported, and srcset is very limited in user experience and optimizations. On top of it, as long as the amp-cache issue is not resolved, pre-loading must be disabled regardless of the selected approach - so srcset loses it's main advantage. However, as mentioned above, we're building the API in a generic way. We'll be able to deliver updates to our extension with different implementations without hurting he users (e.g. if some new form of client hints, supporting both width and height, is widely adopted).

We hope that covers most of the questions raised during the review meeting, please let us know if there are any open issues or comments.

sparhami commented 5 years ago

I'd like to revisit @cramforce's suggestion to create a generic extension. I think this would be a good way forward to maintaining a clean code base without tying this functionality closely to any one provider.

We can then strip off parts of the url transformation (from the config that is mentioned) as client hints become available. It will also prevent the addition of non-generic business logic being added that cannot be expressed with the upcoming client-hints. This will ensure that we can eventually turn this into a plain amp-img down the road.

For example, from the i2i above:

<amp-cld-img
    effect="sepia"
    crop="fill"
    format="auto"
    quality="auto"
    gravity="auto"
>

all seem to be non-generic, and will always require client-side JS to work. This cannot be gradually removed as client-side hints become available. If we were to make this generic, the markup might look like:

<amp-thing
   data-provider="https://..."
   data-extra-params="effect=sepia&crop=fill&format=auto&quality=auto&gravity=auto"
>

or better:

<amp-thing
   src="https://foo.com/blah/img.jpg?effect=sepia&crop=fill&format=auto&quality=auto&gravity=auto"
>

I'm not sure, as-is, the extension code will ever go away here for the proposed approach. Do client-hints support the actual rendered size or just the viewport size? Also, does the proposed code work off rendered size or viewport size (for crop selection)? If it is based on the rendered size, then it seems like client-side JS will always be needed here, which will prevent things like server-side rendering of the <img> tag ever working.

/cc @kristoferbaxter

stale[bot] commented 3 years ago

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