Repository navigation
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
ChrisChV
changed the title
| 2. Thumbnail metadata lives platform-side, not in ``openedx_content`` | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| A new Django model, ``ImageMediaThumbnail``, owned by openedx-platform, is keyed to the **source image**, not to the File component — via a ``ForeignKey`` referencing the source image's ``ImageMedia`` entry (:ref:`openedx-content-adr-0013`), and records which :class:`Media` object is the current thumbnail for that source image, together with any other presentation-adjacent fields the platform needs for that asset. Consistent with Decision 1, this model is a plain, mutable Django row: creating a new thumbnail or changing these fields is a simple update, not a new version. |
There was a problem hiding this comment.
ImageMedia doesn't actually exist. I think I just wrote that as an illustrative example of something I'd like to have down the road.
Also, if it is keyed to ImageMedia, which would be keyed to the immutable Media, why would ImageMediaThumbnail be mutable?
There was a problem hiding this comment.
I also mentioned ImageMedia in #822 and I do think we should build it if we're building thumbnails.
There was a problem hiding this comment.
This idea has been dropped in the new version of ADR.
|
|
||
| Creating or regenerating a thumbnail never creates a new version of the File component. The source image continues to be versioned normally, like any other File component content, but the thumbnail itself is not authored content and does not participate in that versioning. | ||
|
|
||
| 2. Thumbnail metadata lives platform-side, not in ``openedx_content`` |
There was a problem hiding this comment.
It's not clear to me why this should be platform-owned, as opposed to owned by core.
There was a problem hiding this comment.
I've moved it to core in the new version of this ADR
| An independent storage model with no relationship to ``Media`` | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| A platform-side model with its own image field and storage backend, entirely disconnected from ``openedx_content`` — the pattern ``edxval.VideoImage`` already uses for video thumbnails — was considered. It is a simpler design, but it forfeits the content-addressed deduplication that :class:`Media` already provides, and it does not follow the extension pattern that :class:`Media` explicitly documents for third-party apps. |
There was a problem hiding this comment.
Deduplication should be fairly straightforward to do with a disconnected system if we preserved Media's content-based file naming convention, and just appended whatever the resize params were. So the thumbnail file storage could be something like, "{file_namespace}/{original-media-hash}-{thumbnail-params}.webp"
We could even be more aggressive about it and dedupe thumbnails across the whole site, rather than worrying about file storage namespaces.
There was a problem hiding this comment.
Yes, I've followed this convention in the new version of the ADR
|
Another thought for your consideration: We could make an image resizing system that sits completely outside the LearningPackage infrastructure, supports lazy generation, but is also invoked during import/upload. Dedupe can still happen, since we know the source Media file names, and can make a convention of re-using that. Plus we can potentially get much more flexibility, if we ever want to let people tweak the resize parameters. We can even encode the resize parameters into the filename. So if the file exists, we send it back, and if not, we generate it. Image resizing comes up in the context of thumbnails, but it also comes up in a number of other places in courseware, like taking absurdly oversized course card images or HTMLBlock images and resizing them down to something much smaller to send over the wire. |
Adopt the design suggested in review: thumbnails are plain derived
files stored at a deterministic path, with no database model.
- Thumbnails are derived files, never PublishableEntities, versions
or Media rows.
- Generation is a generic API in a new openedx-core app, outside the
learning package infrastructure, called with a variant name.
- Storage path is a pure function of the source Media hash, the
dimensions and an algorithm version:
thumbnails/{media_file_namespace}/{hash}-{w}x{h}-{algo}.{ext}
- Sizes are named variants defined in a Django setting (platform
defaults, overridable per deployment, extended by plugins); the
core app only generates listed variants.
- Thumbnails are generated eagerly on upload/import and lazily on
first request as a fallback.
- Thumbnails are requested through the source file's URL with a
query parameter, reusing its version resolution and permissions.
Replace the ImageMediaThumbnail model with this design and move it to
Rejected Alternatives, together with a dedicated thumbnail endpoint,
arbitrary dimensions and runtime-configurable variants. ImageMedia
becomes an open question.
ChrisChV
changed the title
Thanks @ormsbee, I really like this approach, and I've reworked the ADR around it:
|
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Thanks, I like this overall approach.
|
|
||
| This bounds how many thumbnails can exist for each image, and prevents a request from forcing the generation of an unbounded number of sizes. Because the API from decision 2 only accepts the variants listed in the setting described below, this limit does not depend on each consumer following this convention. | ||
|
|
||
| Variants are defined in a single Django setting, read by the openedx-core app from decision 2. The platform provides the default variants in code, which each deployment can override, and plugins and other apps that use the API add their own to the same setting. Variants are not configurable at runtime (e.g. through the Django admin), because: |
There was a problem hiding this comment.
Variants are defined in a single Django setting
I don't think this should be a Django setting. It's not something that should need to be customized on an instance-by-instance basis. It's not like "MIT wants high res 3x thumbnails, but Harvard wants 2x res". Instead, each variant is defined by the relevant frontend, as you say below, so it's the frontend UI code that should define the variants it needs via the API, not a system setting. But I guess we don't really have a way for (TypeScript) frontends to register with a python API, do we? Hmm, so maybe it does need to be a setting.
Related: I think we should have a default variant that is designed to work with Paragon's <Card> component, and that should then work for most use cases we have: files page, course list on learner home page, searching/browsing for courses, content libraries UI, etc. As much as possible, I'd like everything that's part of the default platform to use this one default, or perhaps two defaults (one cropped, one not). I think a card-sized thumbnail should cover virtually all use cases anyways.
|
|
||
| Thumbnails are stored as plain files, under a path computed from the source :class:`Media` and the parameters used to generate them: | ||
|
|
||
| ``thumbnails/{media_file_namespace}/{source-media-hash}-{width}x{height}-{algorithm-version}.{ext}`` |
There was a problem hiding this comment.
What about cropping? We need to decide if {width}x{height} means "at most {width}x{height}, preserving aspect ratio" or "exactly {width}x{height}, cropping as needed". Usually in thumbnail software, both options are available and specified by the variant, and the cropping or not would also be reflected in the URL.
If cropping, there's also the option of using smart cropping, but that's probably overkill for our needs.
| - The thumbnail for a past version of an :class:`Asset` can be retrieved the same way as the current one, starting from that version's :class:`Media`. | ||
| - Supporting a new size only requires configuring a new variant. | ||
| - Thumbnails are not included in a course's export package, and are regenerated after import. This preserves the behavior of the legacy contentstore. | ||
| - Since there is no database record of which thumbnails exist, serving a thumbnail requires checking whether its file exists in storage. With remote storage (e.g. S3), this check adds latency to every thumbnail request. Eager generation (decision 5) means the file almost always exists, so a request rarely has to wait for a thumbnail to be generated, but it still pays for the check. If this becomes a problem, the check can be avoided later (e.g. by caching which thumbnails exist, or by redirecting to the expected URL) without changing this design. |
There was a problem hiding this comment.
I think this will be a problem, because S3 latency can be significant. We should keep a cache of which thumbnails exist, either in the database or in the redis cache.
Description
This PR adds ADR 0017, "Thumbnails for Images". ADR-0013 drops contentstore thumbnails when course Files are migrated to Assets, and leaves thumbnails to a separate ADR. This ADR defines how thumbnails are generated, stored and served instead. They are derived files, stored under a path computed from the source
Mediahash and the configured size, with no database model. They are generated by a generic API in openedx-core, and requested through the source file's URL.Supporting Information