Negotiate max width/height of oEmbed assets more intelligently

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Tried testing this with 1000X1000 and the image appears zoomed in to me with the patch.

This happens if you set the max_width too high for the region the video is in. I'm not sure what the media module should (or can) do here.

That said, it appears to be a thumbnail issue. I can replicate #35 but when I play the video it fits within the iFrame.

The thumbnail is a fixed width. This was handled in another issue - 🐛 Media thumbnail dimensions are wrong for YouTube videos Fixed .

So this seems to be just Vimeo videos and not affecting YouTube ones. Using a Vimeo video I was able to replicate the same as #31.

Moving to NW for an issue summary to specifically call out what is the proposed solution. Might be worth noting this doesn't affect YouTube, even though the original post mentions this issue was primarily with YouTube videos.

I'm not sure what else I can add to this. The patch does what's described in the updated IS. Google has problems, there are a few issues around that. In this case, the thumbnail is the problem.

Plain embed:

https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=dPiLx...

With maxwidth and maxheight set:

https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=dPiLx...

The thumbnail size doesn't change, but that also doesn't fully explain the problem here.

Plain Google embed without Drupal has the thumbnail problem you describe in #35. Review the following at various browser widths:

https://www.youtube.com/embed/dPiLxdBOnTs

The Oembed spec states thumbnail dimensions must respect maxwidth and maxheight parameters, but clearly this isn't happening with YouTube.

(As an aside, Vimeo seems to handle responsive thumbnails well.)

What do you think we could do here to get around the problem with YouTube's thumbnails? I'd be happy to look into it, but I don't really have any suggestions, other than a theme or JS hack.

No don’t need the thumbnail addressed here but I see the expected behavior in the IS but not what clear what was done to address it.

But if you think that’s not needed can remove the tag and put back into review also

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
@@ -224,8 +224,8 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
-            'width' => $max_width ?: $resource->getWidth(),
-            'height' => $max_height ?: $resource->getHeight(),
+            'width' => $resource->getWidth() ?: $max_width,
+            'height' => $resource->getHeight() ?: $max_height,

This is the key change in this issue and it affects all video providers.

$max_width and $max_height are the values set in the field formatter. These are sent to the oembed provider as the parameters maxwidth and maxheight.

$resource->getWidth() and $resource->getHeight() are the values returned by the video provider, which take into account the values sent with the request.

Before the patch the dimensions of the video were set the values set as $max_width and $max_height.

After the patch, the dimensions are set to the returned values $resource->getWidth() and $resource->getHeight().

It'd be nice to get a few more reviews here.

I've tested patch #27 (can't use patch #34 because it does not apply on Drupal v10.0.x, but code in #34 looks pretty similar). The patch has errors when any dimension field is empty. Error log shows access denied, most likley due missing query parameter values.

Field formatter setting width="1920", height="" causes an error:
Path: /media/oembed?url=https%3A//www.youtube.com/watch.....&max_width=1920&max_height&hash=.....

Field formatter setting width="1920", height="1080" works:
Path: /media/oembed?url=https%3A//www.youtube.com/watch.....&max_width=1920&max_height=1080&hash=.....

#41:

UPDATE: This problem was not introduced by this patch, it also happens on an unpatched v10.0.4

Retested following #31 and got the whitespace and with the patch I did not.

Also may somewhat be related to ✨ Allow oEmbed video iFrames to expand to available space Needs work

The last submitted patch, 34: 2966656-34.patch, failed testing. View results →

Could this be related? 🐛 [random test failure] MediaTest::testLinkManualDecorator() Fixed

Random ckeditor error.

Know spokje has been working on the random failures.

The last submitted patch, 34: 2966656-34.patch, failed testing. View results →

Appears to be unrelated failure.

The patch and fix look good, but I think this needs a release note - this may change the size of embedded video on existing sites when they upgrade?

Hiding patches for clarity

Updated issue summary with release note (not great at those)

Moved patch into MR.

Tests are all green. Since this was previously RTBC going to restore status.

@nod_ has asked a question that needs answering on the MR.

Tried the suggestion but the caused the tests to fail https://git.drupalcode.org/issue/drupal-2966656/-/jobs/1143627

So reverted back.

Let's add the comment @nod_ is suggesting

Added comment.

Thanks

Committed 5559dba and pushed to 11.x. Thanks!

nod_ → closed merge request !7041

For anyone reading the comments via email, the change was also merged to the 10.3.x branch.

Automatically closed - issue fixed for 2 weeks with no activity.