Media sources for local video and audio support

Doesn't the File media source pretty much cover these bases?

I posed this question to @chr.fritsch on Slack, and he made the good point that it could be useful for things like setting the default file extensions of the source field, and so forth. But we could easily accomplish the same thing by giving File a deriver to inject the necessary information into the plugin definition, rather than creating a whole new set of classes.

As for adding more media types to Standard, I think we should hold off, or at least confine that discussion to a follow-up. That kind of thing is usually pretty controversial (due to being user-facing), whereas this is more of an under-the-hood nicety. So let's keep the two discussions separate.

To be honest, I'd like to see a patch that we could iterate on and refine. Right now this is all kind of up-in-the-air...

First question is, do we want something like that? I definitly can see use-cases for that.

I think this makes a lot of sense, the implementation is quite simple, and would contribute to give site builders a lot of flexibility out-of-the box.

And second question is, should we ship media types for that with the standard profile.

I'm not so sure about that though. Maybe that'd be assuming too much, and it could end up being something like the "Body" field that if you don't need you always have to delete from your content types...

We could perhaps start by creating the source plugins, and in a follow-up decide if standard should ship with the types?

We could perhaps start by creating the source plugins, and in a follow-up decide if standard should ship with the types?

+1 for this.

Needs a reroll because #2865184: Allow MediaSource plugins provide default field form/view display settings changed.

Needs a follow-up to debate/implement the addition of local video and audio media types to Standard.

And needs tests because, well, everything does. :)

Working on #8

Re-rolled, implemented the audio source, and added a test for each source.

This is based on #2865184-44: Allow MediaSource plugins provide default field form/view display settings.

Feedback will be appreciated.

This is gorgeous. Looks like a nice, quick win.

+++ b/core/modules/media/src/Plugin/media/Source/LocalAudio.php
@@ -0,0 +1,39 @@
+    return parent::createSourceField($type)->set('settings', ['file_extensions' => 'mp3 ogg wav wma aiff aac']);

Can we support m4a as well?

Also, let's add functional tests of these new media types -- we should be able to prove that we can create a media item of each type, and that they display <video> and <audio> tags as expected.

I have added the m4a extension and changed the plugin ids to file_audio and file_video, maybe these are more future-proof? :)

Concerning the functional test, I was under the assumption that if we can ensure the field is created with the correct type, and its formatter is correctly set to file_audio or file_video, the formatter would do its job (enforced by its own functional test) and we should be good. But it's no big deal really, we can copy what the formatter test is doing and perform a complete test here if you believe it's really needed.

Concerning the functional test, I was under the assumption that if we can ensure the field is created with the correct type, and its formatter is correctly set to file_audio or file_video, the formatter would do its job (enforced by its own functional test) and we should be good. But it's no big deal really, we can copy what the formatter test is doing and perform a complete test here if you believe it's really needed.

The formatter is being tested on file fields that are attached to a node type, I believe; we should test them in the context of a media type too, just to be safe.

No worries! This should cover the functional tests.

Thanks!

"Audio" and "Video" are ambiguous. Let's change the labels to "Audio file" and "Video file", respectively, and maybe the plugin IDs as well to match.

This should fix #16 and #17

I think this looks quite good. I have only various, very minor concerns.

1. +++ b/core/modules/media/src/Plugin/media/Source/AudioFile.php
   @@ -0,0 +1,39 @@
   + * Audio entity media source.
   

   Can we rephrase this? Maybe "Media source wrapping around an audio file" or similar.

2. +++ b/core/modules/media/src/Plugin/media/Source/VideoFile.php
   @@ -0,0 +1,39 @@
   + * Video entity media source.
   

   Same here?

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceAudioVideoTest.php
   @@ -0,0 +1,105 @@
   +   * Check the Local Audio source functionality.
   

   We should probably not say "Local Audio" here, since that's no longer the label of the plugin.

4. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceAudioVideoTest.php
   @@ -0,0 +1,105 @@
   +    $storage = FieldStorageConfig::load("media.$field_name");
   +    $this->assertNotNull($storage);
   +    $field = FieldConfig::load("media.$type_name.$field_name");
   +    $this->assertNotNull($field);
   

   It'd probably be stronger to assertInstanceOf() here, rather than assertNotNull().

5. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceAudioVideoTest.php
   @@ -0,0 +1,105 @@
   +    $this->assertSame('mp3 ogg wav wma aiff aac m4a', FieldConfig::load("media.$type_name.$field_name")->get('settings')['file_extensions']);
   

   Nit: I think we could use getSetting('file_extensions'), rather than get('settings')['file_extensions'] here.

6. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceAudioVideoTest.php
   @@ -0,0 +1,105 @@
   +    $this->assertNotNull($display);
   

   Can we do assertInstanceOf() here?

Thanks for reviewing! This should address #19

I think this looks good. We still need a follow-up as discussed in #8.

Then we can postpone this on #2865184: Allow MediaSource plugins provide default field form/view display settings. Once that lands, this is RTBC.

Agreed!

Created #2930054: [PP-1] Include default media types for local audio and video in standard profile. postponed on this one, and marking this one as postponed on #2865184: Allow MediaSource plugins provide default field form/view display settings

We have a follow-up. Thanks, @marcoscano!

#2865184: Allow MediaSource plugins provide default field form/view display settings landed, so this is not postponed anymore. Marking it RTBC as per #21.

The extensions the media sources accept in the current patch are not supported by all browsers. Once we ship these sources in core and allow certain audio/video extensions, people will expect them to work everywhere. We should at least do one of the following:

• Inform people about the browser support for the allowed extensions.
• Only allow extensions that are supported by all browsers.

Informing people is a start. They should be aware not all browsers accept all formats and that the support is something that changes. This might need input from the usability team as well.

An indication of support for the listed audio extensions:
https://caniuse.com/#search=mp3
https://caniuse.com/#search=ogg
https://caniuse.com/#search=wav
https://caniuse.com/#search=wma
https://caniuse.com/#search=aiff
https://caniuse.com/#search=aac
https://caniuse.com/#search=m4a

An indication of support for the listed video extensions:
https://caniuse.com/#search=mp4
https://caniuse.com/#search=webm
https://caniuse.com/#search=ogg
https://caniuse.com/#search=ogv

That's a good point @seanB, thanks for pointing it out!

It brings an interesting question: What would the average site-builder prefer as default: 1) More extensions, even if some not supported by some browsers, or 2) Less extensions, but all supported everywhere?

I lean towards the second option, probably adding for example only what's currently supported by Firefox, Chrome, Safari and Edge, and let the site builders add others on their own.

I just checked the references metioned in #25 and this would leave us with:

Audio extensions enabled by default:

mp3, wav, aac

Video extensions enabled by default:

mp4

I wouldn't bother adding a warning on the UI, because this would add complexity (where to inform this, how to word it in a way it's always up-to-date, etc), and the process of adding another allowed extension to a file field is very basic, so I think the average user wouldn't have a problem figuring out how to do that.

What do you think?

This is how #26 would look like.

Forgot to update the tests

I agree we should only support formats that work everywhere, but we are going to get questions. Why isn't format X supported. Why can't I upload a .mov / .avi / .ogg etc. When you are not using the HTML5 video player these are all valid questions.

Not sure about the best way to explain this, since we at least need to explain things like the HTML5 video player and browser support. I'm going to tag this for usability review to get some feedback on this.

Why isn't format X supported. Why can't I upload a .mov / .avi / .ogg etc

This doesn't seem like a question that calls for usability review. If site builders want to support those formats, they can configure the source field to allow them (and find a formatter that will play nice with them). If they want core to support those formats, they can post an issue to discuss/argue their case. Doesn't seem like a UX issue to me...but maybe I'm missing something...

Why isn't format X supported. Why can't I upload a .mov / .avi / .ogg etc.

The same argument could be made for "Why core file fields only allow by default .txt files?"

The important nuance here is that all other extensions are supported, they are not just available by default. You would need to deliberately add them yourself.

But I can note this issue to bring it to the UX team to get more feedback if you feel strongly abou it, it won't hurt :)

Thanks!

Well we have certain restrictions in place because of technical limitations (of browsers). Non-technical users will not understand this, I think a lot of technical users don't even know this, so there might be a way to solve this in the interface.
We could solve this with a description to explain this, maybe with a link to online documentation? I'm not sure about the usability aspect of that. But maybe you are right and this is not a usability issue at all.

It's definitely worthwhile to document somewhere -- no question about that. But that's primarily a documentation issue, and -- at most -- a "let's improve some inline help text in core" issue (probably in the File module). I don't think it's a usability thing.

So, if you feel strongly about it, feel free to reinstate the tag; but for now, I'm removing it and kicking this back to RTBC. :)

Speaking from my product manager perspective: focus on the smaller set of formats that are shared across current browsers.

Making more formats available/work: any reason we couldn't rely on contrib to provide that? I expect that even in 2018 people will still be adding contributed modules to their site :)

I think we need explicit sign off from product manager here, @yoroy has commented but that's not the same thing.

I'll ping him and @webchick

That was a testbot hiccup. Back to RTBC.

Testbot glitch

Very minor nit.

+++ b/core/modules/media/config/schema/media.schema.yml
diff --git a/core/modules/media/images/icons/audio.png b/core/modules/media/images/icons/audio.png
new file mode 100755

+++ b/core/modules/media/images/icons/audio.png
diff --git a/core/modules/media/images/icons/video.png b/core/modules/media/images/icons/video.png
new file mode 100755

These files should be 0644 not 0755

Updated the file permissions.

Looks like this patch introduces icons, can we get a screenshot to see how those look and where they're used?

Sure!

Here is product feedback: listing extensions that have wide support is better than trying to list all kinds of extensions but then the user figuring out the hard way that they don't work.

If we can link to a browser support chart that would be nice, but the thing is, the limited list of extensions is on the source field but the supported files would actually be dependent on the formatter. So explaining the formatter we think people use on the source field settings is quite backwards. It is an unfortunate coupling due to browser limitations. Not sure what better can we do though. Because if we add some explanation in the source field then if someone uses a different formatter then we cannot tell what extensions would that support, and people can use different formatters in different instances/view modes/views, so the formatter reaching back to inform the field settings is not really feasible.

All that said, this is the best we can do now from a product standpoint, listing extensions with widest support gets us the most working installation while contrib/formatters can fill in the gaps as @yoroy explained as well. Signed off :)

Did not mean to say no screenshots needed :)

The icons are shown whenever the "Thumbnail" formatter is used.

For example, the Media overview page (/admin/content/media) shows them, and the two we add on this patch look like this:

(This screenshot also shows how a File and an Image media thumbnails would look like).

After creating the media type, the source field is by default at the very bottom. It would be nicer if it's under the media name.

Agreed, but I'm not sure this is in scope. Can you file a follow-up?

Needs work on #48 indeed.

Also why are the new icons filling the whole box while the old text icon is in the middle of the gray box? Some consistency would be important.

Finally, do we know the source of the icons (license, etc?)

Also why are the new icons filling the whole box while the old text icon is in the middle of the gray box? Some consistency would be important.

Finally, do we know the source of the icons (license, etc?)

The icons are png images that take the whole space:

I have no idea about the original authors of the icons or their licenses. In this patch we have used the same icons that the modules media_entity_video and media_entity_audio were originally using.

Maybe to avoid any risks, someone could provide new icons, free to use, that also resemble more the ones we already have?

I personally have no means of producing such graphic material, but if needed, I can help in including them in the patch.

Let's see https://twitter.com/drupalmedia/status/948607187009572866 :)

Re: #48, this also happens with other sources:

With the Media Types pre-packaged in standard it is not the case because the form display has the correct weights, but if the user creates a new file or image Media Type, the source field will be after the "Save" button. So this is not something being introduced on this patch. I have created #2934162: Provide better defaults to media form displays to fix that.

Hm, all right, since we are not shipping with media types for this in this issue, the weight of the field is indeed not in scope.

Definitely should be addressed. Let us open a follow-up for that.

Oop, never mind! I didn't notice #2934162: Provide better defaults to media form displays.

We can definitely help with those icons. How do we go about doing so?

The audio and video icons are definitely not in line with the no-thumbnail and generic icons as show in the screenshot of #51. I guess the biggest issues is the surrounding gray area, but besides that I'm not video thumbnail also seems to introduce a new color.

Definitely not a designer, but I would be happy to voice my opinion if someone has time to create a proposal. @evankay if you have time to work on that it would be very helpful. We are on IRC (#drupal-media) and Slack (#media) as well in case you have any questions.

Here's the link to the icons. Thanks @evankay!

https://www.dropbox.com/sh/a8pi121gcy3kbt3/AAAeeLbx7ynh6b-g6A6HtDzRa?dl=0

Swapped out the old audio and video icons with the new ones linked to in #59. Screenshots before and after the icons were swapped out are attached.

Looks good! Thanks, @starshaped. I think, now that we have incorporated @evankay's custom icons, usage concerns are addressed and this is RTBC once Drupal CI passes it.

Adding issue credit for @evankay's icon work.

This is awesome!

I think the icons might still need to be resized a bit. They're all 180x180, but it does still look like the existing icons have a bit more of a margin than the new ones:

Should they be scaled a bit and have margins added so that they match the existing icons?

The existing icons to match are in core/modules/media/images/icons/:
http://cgit.drupalcode.org/drupal/tree/core/modules/media/images/icons

Ah, the other possibility is that we would also update the existing icons with the ones from https://www.dropbox.com/sh/a8pi121gcy3kbt3/AAAeeLbx7ynh6b-g6A6HtDzRa?dl=0. Then they would have the same dimensions. :) So maybe it is okay and we just need a followup issue. (The set is missing a replacement for the "no icon"one but we could handle that in the followup too.)

Discussed with @xjm. We agreed to proceed with this patch as-is, then clean up the icons in a follow-up. Which I filed: #2934960: Clean up icons that ship with Media.

So: full steam ahead!

Also opened #2934962: Ship local audio and video media types in Standard at @xjm's request.

Thanks @phenaproxima; I agree with handling those two things in the followups.

1. +++ b/core/modules/media/src/Plugin/media/Source/VideoFile.php
   @@ -0,0 +1,39 @@
   +/**
   + * Media source wrapping around a video file.
   + *
   + * @see \Drupal\file\FileInterface
   

   At first I thought the @see was a copypaste error, but actually, for core's purposes, video and audio really are just specific cases of files with certain extensions.

2. +++ b/core/modules/media/src/Plugin/media/Source/VideoFile.php
   @@ -0,0 +1,39 @@
   +class VideoFile extends File {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function createSourceField(MediaTypeInterface $type) {
   +    return parent::createSourceField($type)->set('settings', ['file_extensions' => 'mp4']);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayInterface $display) {
   +    $display->setComponent($this->getSourceFieldDefinition($type)->getName(), [
   +      'type' => 'file_video',
   +    ]);
   +  }
   +
   

   So File, which this extends, has:

     /**                                                                           
      * {@inheritdoc}                                                              
      */
     public function getMetadataAttributes() {
       return [];
     }
   

   But videos do have lots of relevant metadata like width, height, frame rate, etc. However, as @phenaproxima pointed out, getting that metadata would require something like ffmpeg. So for the core patch, the simple implementation is best. (Richer metadata support could be provided in contrib.) The same all goes for video thumbnails.

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceAudioVideoTest.php
   @@ -0,0 +1,105 @@
   +    $assert_session->pageTextContains("$type_name Audio media asset has been created.");
   +    $assert_session->elementExists('css', "audio > source[type='audio/mpeg']");
   

   At first I was mystified as to how this worked. Because it does work; I also manually tested and confirmed that it works. But there's nothing in the patch adding the formatters.

   That's because @webchick already committed them in #1174892: File field formatters for rich media display with <video> and <audio> HTML5 elements. the last time I was in Boston. ;)

All in all this is actually a pretty clean and simple addition now that we have the underlying architecture.

Committed and pushed to 8.5.x. Yayyy!