Add poster image to HTML5 media videos

Rerolled patch.

Rerolled patch with git format.

@pbouchereau the patch needs to be based in the docroot, not within core

@sleitner
And without a missing file too... learning the hard way.
Thanks.

The last submitted patch, 48: 2954834-48.patch, failed testing. View results →

phpunit has been updated in DrupalCI to 9.6.1 and throws new deprecation warnings. Not related with your patch as far as I can see.

Branch testing only failed temporarily.

The last submitted patch, 48: 2954834-48.patch, failed testing. View results →

Branch testing only failed temporarily again.

updated the issue summary.

Wondering if this needs a review / sign off from a 'subsystem' media maintainer. Already seeing some of them commenting.

Added some tags so hopefully it will reach more people, searching the issue queue :)

----

btw really liking this new feature

making clear thee current patch is about mapping the fields at a media entity.

The last submitted patch, 48: 2954834-48.patch, failed testing. View results →

Branch testing only failed temporarily again.

The last submitted patch, 48: 2954834-48.patch, failed testing. View results →

Branch testing only failed temporarily again.

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
@@ -66,6 +92,22 @@ class FileVideoFormatter extends FileMediaFormatterBase {
+        '#type' => 'select',
+        '#title' => $this->t('Poster field'),
+        '#description' => empty($image_fields) ? $this->t('Please add an image field to this media entity to use the poster feature.') : NULL,
+        '#default_value' => $this->getSetting('poster'),
+        '#options' => $image_fields,

The text here shouldn't use 'Please', I think we have an issue around to remove please from the code base entirely. Also not sure 'poster feature' helps much.

Maybe:

'An image field to use as the source of the poster attribute'?

--

Also what happens if the poster image uploaded is wildly different dimensions to the video - does this need to support image styles? Will add some complexity to the patch to do so, but was also mentioned in #30.

We have tested this patch for over a month now and I agree with catch. without an image style it is not usable for content-editors.

For site builders the option list is getting really long now and with adding image style options inside this screen I think we need some extra structure like fieldsets/details to group some settings. (also used an other patch to add some other video settings)

As we're now running into this requirement, I'll create a MR from the latest patch and will try to work on this. Really helpful addition.

Yay, it basically works!! :)
Give it a try on the tugboat preview.

Now I'm wondering, if the implementation currently in viewElements() shouln't at least partially be moved into prepareAttributes() or what's the reason for the decision to put it into viewElements().
Also I'm not totally sure about the Cache tags implementation.

Would be nice to have another review, I think we're making progress here and I'm happy to learn details. Also we should have a look at the existing tests again, I didn't touch them yet.

Current status:

@Grevil please note the 9 upstream test fails in core, that will also fail here, until it's resolved in core.

I added a few comments, otherwise LGTM!

Forgot to mention it. I debugged the cache tags and they also LGTM!

Thanks @Grevil, I fixed the image style test now, please review finally.

Switching back to 10.1.x so we hopefully get this in before stable as minor fix.

See https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... → (linked in the comment above) 11.x is the target branch for everything now and we backport from there.

Thanks @catch!

GitLab rebase didn't work so let's create a fresh new MR against 11.x instead and leave the 10.1.x one if someone uses / needs it in the meantime? @Grevil could you do that?

So the old MR might still be reviewed in the meantime to speed things up.

Testing the new image style feature.

A user has now 3 options within the poster field:

• None
• Thumbnail
• Image

As showed below the thumbnail option and the image option are equal. With both a user can select an image style. So I was wondering what is the difference?

Thank you for the feedback @Martijn de Wit. This shows possible confusion for end-users.

As showed below the thumbnail option and the image option are equal.

It's not the same. While in the "Poster field" you pick the image field to display as poster.
In the "Poster field image style" field, you select the image style (Imagecache was the name in Drupal 7) to apply on the image.

You say the options are equal - but that's not the case and it doesn't look like this is true on your screenshots?

Perhaps the UX team should have a short look to improve the descriptions or field labels?

Added a new branch "2954834-add-poster-image-transcript-from-fields-11.x" and applied the current "MR !4010 " diff.

@Anybody, as you created the MR, you should (hopefully) be able to close it again. I get a 404 error, when trying to close the old MR.

A yeah, now I get it :)

I have 2 image fields on my video entity. one named 'image' and one named 'thumbnail' from previous tests.
Sorry for the confusing post.

Then it works as advocated, so it was a good test :)

Took a look and believe we will need an upgrade path for the new formatter settings.

Thanks @smustgrave! That would mean, we need an update path to set all values to empty on existing formatters:

      'poster' => '',
      'poster_image_style' => '',
      'transcript' => '',

Is that really needed? I guess if yes, it's because the keys have to be added?
Do you know any similar example we could use for the update hook?

Would it perhaps make sense to add a helper for such cases to make field (formatter / storage) settings updates more simple (DX)? ✨ Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed) Needs work
At least to me such things always feel quite hard to do it right, having to copy existing examples etc... we shouldn't open that side topic here, I guess, but perhaps this is a good starting point to proceed the discussion in the linked issue or a new meta issue?

May be a good example https://www.drupal.org/project/drupal/issues/3309016 📌 Add image preload option to help boost actual and perceived performance Needs work

Is that really needed?

Yes we need to update existing configuration so that if you update and do a config export, the exported configuration matches how things would look if you'd resaved. If we don't do this, then when you save formatter settings later manually, these keys would be added which gets very inconsistent/confusing.

The pattern is to put the logic for adding the new keys into a presave hook, and then using the actual post update function to just load and re-save configuration. Views (both in 10.1 and 9.5) has the most examples of this, but also a grep for ConfigEntityUpdater will find more.

Anybody → closed merge request !4010

I added an update hook, but I couldn't figure out, how to specifically target field instances using the "file_video" formatter. Please review and adjust accordingly!

May be worth asking #contribute channel.

But the version number wouldd be 101021

The update should use hook_post_update_NAME() rather than hook_update_N(), and the logic of actually updating the configuration should happen in hook_entity_presave(), not in the update function itself, so that it also runs when importing default config and similar.

Thank you, @catch!

I got the point on using the hook_post_update_NAME() hook rather than the hook_update_N() hook, but didn't understand the hook_entity_presave() part, as we are basically only updating a few "entity_form_display" setting values and do not change the config itself. So why exactly the "hook_entity_presave" hook? Am I missing something? Is there an example with a similar update in core?

For an example of another update, see:

/**
 * Implements hook_ENTITY_TYPE_presave() for entity_view_display.
 */
function media_entity_view_display_presave(EntityViewDisplayInterface $view_display): void {
  $config_updater = \Drupal::classResolver(MediaConfigUpdater::class);
  assert($config_updater instanceof MediaConfigUpdater);
  $config_updater->processOembedEagerLoadField($view_display);
}

Which is related to:

media_post_update_oembed_loading_attribute()

Grepping for ConfigEntityUpdater will find more post updates which will usually be tied to a presave hook in the respective module.

Looking at this again, I think we might need to do more with the transcript support though:

HTML allows multiple <track> elements, to support subtitles in different languages as well as captions, see the examples on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/track

So... should we think about supporting a multiple value field for 'track'? But then this would ideally support the 'kind' attribute as well as language etc., it is starting to look like another media type with its own fields at that point though and then a multi-value media field on the video entity.

Tagging for needs subsystem maintainer review to hopefully get a second opinion on that last bit.

Thanks for the clear-up @catch! I'll have a look again, once everything is discussed with the relevant sub system maintainer! :)

I think this might be worth splitting into two issues - one for poster image for which I assume there should just be one.

And one for transcript/<track> where there are more possibilities - if we do split, that should probably be the new issue since there'll be more to figure out on there. Might be worth waiting to see what media maintainers say though.

@catch totally agree with #94, thought the same yesterday. And I think the poster image is much more important / widely used. Google Webmaster Tools complains, if it's missing, so we should get that one in soon.

Yes, let's wait for media maintainers feedback.

HTML allows multiple
elements, to support subtitles in different languages as well as captions, see the examples on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/track

So... should we think about supporting a multiple value field for 'track'? But then this would ideally support the 'kind' attribute as well as language etc., it is starting to look like another media type with its own fields at that point though and then a multi-value media field on the video entity.

Since media entities are translatable, I don't think we need to support different tracks for different languages. You probably only want the track for the current language (like how you would handle translations for other entities).

The kind attribute seems interesting, although I'm not completely sure if we can properly explain the different use-cases to site editors. I think accessibility experts might have more insights on when / how to use the kind attribute properly. It would already help a lot to configure the kind attribute on the formatter. Site builders can add a field description on the field where editors can upload the .vtt files, to make sure their editors understand what kind of input is expected.

Since media entities are translatable, I don't think we need to support different tracks for different languages. You probably only want the track for the current language (like how you would handle translations for other entities).

I don't think this is right for subtitles. You can have a video in a single language, with subtitles in 15 languages, and as someone watching the video, you would want those subtitles options to be available via the video player, not by having to view different language versions of the entity. i.e. you can watch an English language film with English, English captions, French, or Arabic subtitles. Even if someone is browsing a site in French, and there is a video that only has English audio, they still might want to choose English subtitles instead of French (i.e. if they're trying to learn English and want to pause and read sometimes when it's hard to hear).

I guess you are right we probably need to support multiple subtitle files in each translation, since the source field could technically be different for each translation. So you could have a different video for each language, while still wanting subtitles in multiple languages for each translation.

Then we only need to decide how to deal with the kind attribute. I don't feel I'm the best person to make a decision here. At the moment, I'm thinking it is already helpful to be able to configure a single kind value on the formatter for all the files uploaded in the selected file field. That should also be relatively easy to implement. If we need to support multiple kind attributes, I think we need to be able to configure a different file field for each kind. This is a bit more complex, but if we need to support this I would probably vote to do it like this.

Thanks for the discussion! I think we should split this then to not block the poster image implementation, which Google Search Console complains about. This is the key feature most users need.

Implementing transcript is also helpful, but not that widely used, I think. So I'd suggest

1. We change the title of this issue to "Add poster image to HTML5 media videos"
2. We create a sibling issue: "Add transcript to HTML5 media videos" as follow-up and postpone it on this one
3. We split the code so that this can be reviewed and merged and work on the transcript feature can be proceeded in detail later on.

Would have been better we did this earlier, sorry.

Yes splitting is good, but I think we should use track for the new issue rather than transcript, since that covers the transcript vs. subtitle use-cases and the HTML element is the same for both.

Created the child issue here: ✨ Add track to HTML5 media videos Postponed .

Alright, I removed all the transcript parts. Please review, in case I missed anything!

Also, do we still need to implement the hook_ENTITY_TYPE_presave(), as discussed in #90 and #91? Still unsure about that bit.

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

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rebased

Seems we are missing upgrade test coverage. Should be simple though to add.

Updated the patch from #48 ✨ Add poster image to HTML5 media videos Needs work to include media entity reference field for thumbnail image on Drupal 10.x

Updated the patch from #48 ✨ Add poster image to HTML5 media videos Needs work to display default image from selected poster image field, if no thumbnail is added to video media.

Updating the patch in #109 →
Fix issue if no default image is added to field.

Why using the patch from #48?

There is a merge request that is far more recent? containing solution after several discussions with good points ..

This patch only for poster attribute.
1. Once the patch applies, then add existing image field
2. Change the poster option as an image in Manage display in Video Media type

Resolved merge conflicts after 📌 Convert FieldFormatter plugin discovery to attributes Active was committed.

Hiding all old patches