-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
I2I: Cloudinary amp extension #21500
Comments
Hi @nitzanj, thanks for filing this! A couple of clarification questions:
A few implementation considerations:
I think my questions boil down to: Thank you! |
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.
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 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.
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.
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.
This is explained above under the advanced features.
As explained above, the proposed solution is probably not something that Thanks! |
Got it. Thank you very much @nitzanj for the explanation! This sounds great.
Wow.
Nice! Two more things to possibly think about:
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! =) |
Hi @nitzanj! I wanted to check in and give you a heads up on the following:
Again, feel free to reach out if you need help or have questions. =) |
For the extension name, I suggest |
LGTM, too. Note that these will not be pre-renderable images, because we won't be able to deliver them via |
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. |
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. |
I'd like us to consider a view things:
|
@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: Type: Design Review 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. |
One concern that came up is that, because this does not use |
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?
These transformations could happen on the various
If the implementation was entirely server driven, the generated URLs could still be serviced by the Cloudinary services allowing for the permutations as expected. |
Following the design review, here are the main takeouts:
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. 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. |
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 /cc @kristoferbaxter |
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. |
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:
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
The text was updated successfully, but these errors were encountered: