Allow HTML5 video/audio formatters for link fields on Media entities

This would be indeed great to support. It won't be possible to simply extend the existing formatters as they assume files and an instance of entity reference field (getEntitiesToView() and such). My suggestion would be to move common code into a trait and create another set of formatters for links.

Attached patch is far from complete but shows what the idea is. Thoughts?

Re-rolled against 8.6.x.

This patch leaves FileMediaFormatterBase but has it use the new trait.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/MediaFormatterBase.php
   @@ -0,0 +1,80 @@
   +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
   +    // TODO see if we can hide formatter for irrelevant fields to improve UX.
   +    // Maybe limit only to Media entities?
   +    return TRUE;
   +  }
   

   For link fields I don't see how we could know whether the Video/Audio formatter is applicable or not.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/MediaFormatterBase.php
   @@ -0,0 +1,80 @@
   +      $mime_type = \Drupal::service('file.mime_type.guesser')->guess($link->uri);
   +      // TODO some streams are application/octet-stream and possibly others?
   +      if (TRUE || static::mimeTypeApplies($mime_type)) {
   

   As @Berdir points out streams don't have file extensions but the MimeTypeGuesser was able to guess the mime type for an extension-less stream URL (http://audio.wgbh.org:8000). That said...

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/MediaFormatterBase.php
   @@ -0,0 +1,80 @@
   +        $source_attributes
   +          ->setAttribute('src', file_url_transform_relative($link->uri));
   +          // TODO wrong type will fail to play stream even if playable w/o
   +          // the type attribute. Could we skip?
   +          //->setAttribute('type', $mime_type);
   

   When testing with an audio stream URL the mime type guesser guessed "application/octet-stream" but when used as the "type" attribute on the element the audio wouldn't play. Removing the type attribute allowed the stream to play. I'm thinking, for link fields at least, we omit the type attribute. (In my testing the type attribute is not even reliable when you do have a file extension: see #2940065: File field formatter for video doesn't work for .mov files)

Are there fields other than links and files which could use an audio or video formatter? I'm thinking the MediaFormatterBase, VideoFormatterBase and AudioFormatterBase added in the patch should be renamed LinkMediaFormatterBase, LinkVideoFormatterBase and LinkVideoFormatterBase.

Trying to move this forward.

Sorry for the big interdiff, but in this version, I have:
- Renamed and moved the audio/video formatters to the media module namespace. Also, renamed some ids to clearly indicate that these formatters apply to streams.
- Made them applicable only to links inside Media entities. This is probably a debatable decision, and the reasoning behind it is that these formatters are not likely to be used a lot, and would pollute all link formatter settings in the system otherwise.
- Added the missing schemas
- Fixed some minor CS violations and TODOs
- Created 2 new media source plugins, which would use link fields (with the appropriate formatters by default) as source (instead of file fields)
- Started some test coverage

Still to discuss:
- There was a TODO left in the code about what to do when the MIME guesser couldn't reliably identify the MIME type of the stream. But once noted in #9, maybe it is not necessary at all if the type is not included as an attribute? Why is it necessary in the first place?

Sorry for polluting the issue, I was temporarily unable to run tests locally.. but all set now.

(sorry, uploaded the wrong file)

@Berdir thanks for reviewing!

1- Yep, I see both points of this too. I feel though that exposing this formatter (which is kind of very specific) to all link fields would be too much... Maybe we could think of adding a setting to the field config, so the user can allow the formatter on a per-field instance? Or some sort of global setting, disabled by default, that would also extend this to all link fields...

2- Yeah the naming is kind of weird for streams, but we are reusing a lot of the file_* core formatters, so in that context it kind of makes sense, especially for remote files.
I've added a comment and a test for the file_url_transform_relative() stuff.

3- it turns out that
\Drupal::service('file.mime_type.guesser')->guess('https://archive.org/download/testmp3testfile/mpthreetest.mp3')
returns audio/mpeg
and
\Drupal::service('file.mime_type.guesser')->guess('http://bbcmedia.ic.llnwd.net/stream/bbcmedia_radio2_mf_p')
returns application/octet-stream (which I believe is the fallback for a non-guessed type).
That said, in this version of the patch I dropped the type attribute that was present in an earlier version of the patch, once as mentioned above, when it is present but doesn't match the real file type, the audio player doesn't work. Was the type attribute necessary for some reason?
In case that was necessary, I have no idea though if the guesser needs to download the entire file to guess the mime type...

4- Thanks for pointing that out, there was actually a bug in the muted handling... Should be all covered now.

Thanks!

1. +++ b/core/lib/Drupal/Core/Field/MediaFormatterTrait.php
   @@ -0,0 +1,123 @@
   +/**
   + * Useful methods for media formatters.
   + *
   + * @see \Drupal\file\Plugin\Field\FieldFormatter\FileMediaFormatterBase
   + */
   +trait MediaFormatterTrait {
   

   "media formatters" is a vague term. Can we expand on what that means? Specifically, this trait is for formatters which deal with media *files* that are viewable/listenable with the <audio> and <video> tags. Therefore, I'd also suggest renaming the trait to MediaFileFormatterTrait.

2. +++ b/core/lib/Drupal/Core/Field/MediaFormatterTrait.php
   @@ -0,0 +1,123 @@
   +    return static::getMediaType();
   

   Should this be an abstract method in the trait?

3. +++ b/core/lib/Drupal/Core/Field/MediaFormatterTrait.php
   @@ -0,0 +1,123 @@
   +   * @param string[] $additional_attributes
   +   *   Additional attributes to be applied to the HTML element. Attribute names
   +   *   will be used as key and value in the HTML element.
   

   The comment needs rephrasing -- these are boolean attributes that are only applied if getSetting() has that attribute name set to a truthy value.

4. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -40,6 +40,24 @@ field.formatter.settings.media_thumbnail:
   +    muted:
   +      type: boolean
   +      label: 'Muted'
   

   Does this not apply to the audio formatter too?

5. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaStreamFormatterBase.php
   @@ -0,0 +1,90 @@
   +    return ($field_definition->getTargetEntityTypeId() === 'media');
   

   Nit: The parentheses are superfluous.

6. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaStreamFormatterBase.php
   @@ -0,0 +1,90 @@
   +      if ($this->getSetting('multiple_file_display_type') === 'tags') {
   

   I wish that this could be a constant, but the fact that we're using a trait makes that impossible (I believe).

7. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/VideoStreamFormatter.php
   @@ -0,0 +1,94 @@
   +      'width' => [
   +        '#type' => 'number',
   +        '#title' => $this->t('Width'),
   +        '#default_value' => $this->getSetting('width'),
   +        '#size' => 5,
   +        '#maxlength' => 5,
   +        '#field_suffix' => $this->t('pixels'),
   +        '#min' => 0,
   +        '#required' => TRUE,
   +      ],
   +      'height' => [
   +        '#type' => 'number',
   +        '#title' => $this->t('Height'),
   +        '#default_value' => $this->getSetting('height'),
   +        '#size' => 5,
   +        '#maxlength' => 5,
   +        '#field_suffix' => $this->t('pixels'),
   +        '#min' => 0,
   +        '#required' => TRUE,
   +      ],
   

   #min should probably be greater than 0. A 0x0 video isn't much good to anyone :)

8. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/VideoStreamFormatter.php
   @@ -0,0 +1,94 @@
   +    $summary[] = $this->t('Size: %width x %height pixels', [
   

   I think "Size:" is superfluous here.

9. +++ b/core/modules/media/src/Plugin/media/Source/AudioStream.php
   @@ -0,0 +1,45 @@
   + *   allowed_field_types = {"link"},
   

   Can/should we also support string and string_long fields? If so, I'm OK deferring that to a follow-up if you want. The problem is that, if the Link module is not enabled, the source field will not be create-able (or even work correctly). It might be preferable for us to alter the audio_stream and video_stream plugin definitions and make them use link fields if and only if the Link module is enabled.

10. +++ b/core/modules/media/src/Plugin/media/Source/AudioStream.php
    @@ -0,0 +1,45 @@
    +      ->create([
    +        'field_storage' => $storage,
    +        'bundle' => $type->id(),
    +        'label' => $this->pluginDefinition['label'],
    +        'required' => TRUE,
    +      ]);
    

    I think we should probably do something like "$label URL", since we are in fact expecting a URL here.

@phenaproxima thanks for reviewing!

1- OK

2- Sorry, not sure I get you... could you please elaborate a bit on that?

3- OK

4- The tag allows it, but for some reason, core's audio formatter for local files doesn't include it either: http://cgit.drupalcode.org/drupal/tree/core/modules/file/config/schema/f... . Maybe in the name of consistency we should do the same here, and decide later if both need to be changed to include the "muted" setting for audios too?

5- OK

6- I guess so too. We could possibly make this constant live elsewhere, but I guess it's not worth the confusion...

7- Changed them to 1. While a video of 1x1 still doesn't make much sense, I think it's not worth starting a discussion about "what the minimum reasonable size for a video should be" in this case... :)

8- OK

9- I'd rather postpone supporting these other fields to a follow-up. In this case, I believe this is a rather specific functionality and I'm not sure exposing the formatter to all those other fields as well adds much value. I guess nowadays most sites will have the Link module enabled (once it comes enabled by default on standard, and also other modules depend on it, such as "Custom Menu Links", "Menu UI", "Shortcut", etc). If a site didn't have it enabled and creates a Media type using this source, we can maybe just enable it before creating the source field? Maybe something like this (untested):

class AudioStream extends File {

  /**
   * {@inheritdoc}
   */
  public function createSourceField(MediaTypeInterface $type) {
    if (!\Drupal::moduleHandler()->moduleExists('link')) {
      if (\Drupal::service('module.installer')->install(['link'], TRUE)) {
        drupal_set_message($this->t('The @source_label media source depends on the Link module, which has been enabled.', ['@source_label' => $this->pluginDefinition['label']]));
      }
    }
    $storage = $this->getSourceFieldStorage() ?: $this->createSourceFieldStorage();
    return $this->entityTypeManager
      ->getStorage('field_config')
      ->create([
        'field_storage' => $storage,
        'bundle' => $type->id(),
        'label' => $this->pluginDefinition['label'],
        'required' => TRUE,
      ]);
  }

//...

}

10- OK

Re-rolled the patch against HEAD and updated the IS with some more detailed explanation and screenshots.

Also, tagging for usability review.

Turns out the failure in #29 was due to the oembed formatter bug after all. Re-rerolled now that it was reverted.

To be honest I feel this is probably something for a contrib module. I think most sites will not use this, personally I have only had to make something similar once or twice. Not sure how we can properly check this? Maybe if the contrib get's a huge number of installs we could evaluate it again?

I also think having more audio/video options (next to local files and oEmbed) by default could be confusing. This also makes me doubt whether we should add this in core at the moment.

I think most sites will not use this, personally I have only had to make something similar once or twice.

I agree to some extent :). But an argument could also be made that it is equally "uncommon" to need to upload an audio file to the server, and core currently supports that out-of-the-box, even with a pre-baked media type. I personally would even go a little bit further and adventure to guess that it is likely more probable that you need to render a video tag from a remote video URL rather than upload mp3 files to your server...

That said, I agree with #34 in that, regardless if in core or contrib, the UX confusion will happen with the current naming...

#34
The choice was made to handle remote video's through oEmbed. Since oEmbed would probably not mean anything to most users naming the youtube/vimeo derivative 'Remote video' was a logical choice. At least core should be easy to understand and use by default. I can see that could be confusing when adding non-oEmbed types of remote video. Since naming things is not my speciality I'm not really sure what would be the best way to fix this. Maybe the new module could also rename the core source when adding a new one to make the difference more clear?

The local video/audio fields are relying on local file pretty heavily, so I guess a new source would be the way to go. The main reason we are not using link fields in core media at the moment is because we didn't want the link module to be a dependency. We would not be using most of the functionality for link fields anyways. Contrib modules would be free to choose link/file fields and support them, but I don't think we should make the core audio/video source do that.

#35
I agree uploading local files/videos would not be a very common thing, but since we were trying to create parity with core file fields that seems to make a lot more sense out-of-the-box. Not sure about the remote video URL vs local mp3, but I see your point :)

About the UX confusion, when adding another remote video source I can see there is an issue. See my reply to #34 for a possible solution. Hopefully we don't have to make it more complicated for the 80/90% use case that would never have this problem in the first place.

Re-rolled #32, and looks like it working fine on 8.9.13

Rerolled #32 for 9.2.x.

+++ b/core/modules/media/src/Plugin/media/Source/AudioStream.php
@@ -0,0 +1,45 @@
+ *   allowed_field_types = {"link"},

+++ b/core/modules/media/src/Plugin/media/Source/VideoStream.php
@@ -0,0 +1,45 @@
+ *   allowed_field_types = {"link"},

I think we can use UriItem (uri) here instead of LinkItem (link).

1.

-  public static $modules = [
+  protected static $modules = [
     'link',
   ];

we are not dependent on link module. UriItem is in core.
2.

+ *   field_types = {
+ *     "link"
+ *   }

3. This should be uri now.
+ type: file.formatter.mediaThis can just be mapping now.

Seems it is not gonna make its way into Drupal core. I've just published a little module based on the patch from this issue.
It also supports uploading local images for video posters.
https://www.drupal.org/project/media_stream