xwp / wp-core-media-widgets

Feature plugin for development of new core media widgets for image, video, and audio (and others)
https://wordpress.org/plugins/wp-core-media-widgets/
35 stars 8 forks source link

VideoPress Uploads Not Functioning #160

Closed timmyc closed 7 years ago

timmyc commented 7 years ago

I tested out the flow of using the Video Widget along side a site running Jetpack/VideoPress, and currently it is not working seamlessly

Steps 1- Install Jetpack and a plan that includes VideoPress 2- Via the customizer, create a new Video Widget 3- Upload a new video to the library, note the preview in the modal will show a Jetpack Video Processing image 4- It seems after clicking Add to Widget, the media items post URL is returned and set on the widget instance, as such the preview in the widget and in the customizer will not work

Alternatively, uploading a video via the Media Library directly, then selecting it within a Video Widget from the library works fine.

westonruter commented 7 years ago

@timmyc do you think it's a bug with how VideoPress extends Media?

timmyc commented 7 years ago

I didn't have a chance to dive in fully on the issue yesterday, but I do think it is specific to how the plugin processes the video off-site, and until the processing final URL is assigned, it uses the attachment URL instead.

I'm not aware of other plugins that do the same sort of thing to media uploads, but imagine there might be others doing this.

I did get a test env setup where I can debug this a bit further today though.

melchoyce commented 7 years ago

@dbtlr is going to take a look at this tomorrow.

dbtlr commented 7 years ago

I've been digging a bit, and it seems like the main issue here is that VideoPress uses a special mime-type to delineate it's attachments of video/videopress the reason for this is because these media objects are virtual and are not represented by local files.

What this means is that the uploads are working fine, but what is failing is the ability to view the VideoPress media items afterward. In some way, we need to add on video/videopress to the form post, like so: query[post_mime_type]:video/mp4,video/mp4,video/webm,video/ogg,video/x-ms-wmv,video/x-flv,video/videopress. After that, as far as I can tell, videopress videos should be selectable.

I've identified a fix that we can add to Jetpack, which fixes this issue. I'm waiting on that PR to be reviewed, however VideoPress videos are available in the selector once this is done.

One note: processing of videos can be slow and happens in the background. Ideally, we should be able to have a hook that we can override the player that is shown in the customizer to instead show the VideoPress player, which responds well to the loading state (which can take from 30-seconds to 5-minutes, depending on the size of the uploaded file.)

Basically, we need to be able to add an iframe instead of a video, when the attachment url looks like this: https://videos.files.wordpress.com/OAVHZIvv/bunny_small_hd.mp4

The src for the iframe would then become this: https://videopress.com/v/OAVHZIvv.

I've messed with a couple ways to do this, but they all seem way too complicated. Ideally, the videopress_guid property that is returned by the API would be detected and then used to create the iframe.

Open to helping add this in if that is worthwhile. I don't really see a good way to do this from Jetpack, so I'm guessing it needs to be hard coded here.

Hope that helps!

westonruter commented 7 years ago

@dbtlr thanks for the research.

What this means is that the uploads are working fine, but what is failing is the ability to view the VideoPress media items afterward. In some way, we need to add on video/videopress to the form post, like so: query[post_mime_type]:video/mp4,video/mp4,video/webm,video/ogg,video/x-ms-wmv,video/x-flv,video/videopress. After that, as far as I can tell, videopress videos should be selectable.

This should be done automatically once you implement https://github.com/Automattic/jetpack/pull/7167 since then then the video/videopress MIME should appear among wp.media.view.settings.embedMimes and this is then what is used to determine which videos are listed in the media browser.

One note: processing of videos can be slow and happens in the background. Ideally, we should be able to have a hook that we can override the player that is shown in the customizer to instead show the VideoPress player, which responds well to the loading state (which can take from 30-seconds to 5-minutes, depending on the size of the uploaded file.)

You can see the template which is used to render the video preview in the control here: https://github.com/xwp/wp-core-media-widgets/blob/811779fb6f97a782cd877e859be97e5cd74f97a2/wp-includes/widgets/class-wp-widget-media-video.php#L230-L252

In particular, when there is a src it will pass through into wp_underscore_video_template(). It seems like there should be a condition that is injected in-between these two cases for custom handling of a video:

<# } else if ( data.is_hosted_embed ) { #>
    <a href="{{ data.model.src }}" target="_blank" class="media-widget-video-link no-poster">
        <span class="dashicons dashicons-format-video"></span>
    </a>
<# } else if ( data.model.src ) { #>
    <?php wp_underscore_video_template() ?>
<# } #>

One way to do this, which would certainly not be the most elegant, would be to add some JS to the page which modifies the template, doing something like this:

jQuery( function() {
    $( '#tmpl-wp-media-widget-video-preview' ).each( function( template ) {
        var templateContents = $( template ).text(), videoPressCondition;
        videoPressCondition = '<# } else if ( /^https:\/\/videos\.files\.wordpress\.com\/.+\.mp4/.test( data.model.src ) ) { #><iframe src="https://videopress.com/v/{{ data.model.src.replace( \'https://videos.files.wordpress.com/\', \'\' ).replace( \'/.+\', \'\' ) }}">';
        templateContents = templateContents.replace( /(?=<# } else if \( data.model.src \) { #>)/, videoPressCondition );
        $( template ).text( templateContents );
    } );
} );

If this is a common enough use case, perhaps support for this should be added to PHP itself with something like:

<# } else if ( data.is_hosted_embed ) { #>
    <a href="{{ data.model.src }}" target="_blank" class="media-widget-video-link no-poster">
        <span class="dashicons dashicons-format-video"></span>
    </a>
<?php echo apply_filters( 'media_video_widget_preview_template_condition', '' ); ?>
<# } else if ( data.model.src ) { #>
    <?php wp_underscore_video_template() ?>
<# } #>

Not the most elegant either, but it should work.

timmyc commented 7 years ago

@dbtlr at what point in the upload process does the unique ID ( i.e. OAVHZIvv in your example above ) get generated and persisted in the attachment?

dbtlr commented 7 years ago

@timmyc The GUID is generated immediately on upload and the video is queued up to be processed by our transcoders in the background.

westonruter commented 7 years ago

@dbtlr will hacking the template as shown in https://github.com/xwp/wp-core-media-widgets/issues/160#issuecomment-300702826 get a workable solution for ensuring VideoPress works in 4.8? Thoughts on a more elegant way to extend the template?

dbtlr commented 7 years ago

@westonruter I don't think this is going to work, as we need to pull the GUID out of the src url not the filename.

We need to take a url like: https://videos.files.wordpress.com/OAVHZIvv/bunny_small_hd.mp4 and match the OAVHZIvv.

What would be ideal, as I mentioned previously, would be to use the videopress_guid that is returned by the media endpoint on sites with VideoPress enabled. If this property is detected, then we can be certain that this is a VideoPress video.

Otherwise, I'm not a huge fan of hacking the template like this. I would prefer this to either be a first-rate condition OR for there to be some sort of filter that can be used to decide how this works.

The obvious first rate condition would be for the template to have a data.iframe_src that could be provided to it just the way data.is_hosted_embed is provided. This would provide for more ways in the future to also add other embeddable media sources here.

westonruter commented 7 years ago

@dbtlr

I don't think this is going to work, as we need to pull the GUID out of the src url not the filename.

Isn't the source URL with the GUID (https://videos.files.wordpress.com/OAVHZIvv/bunny_small_hd.mp4) inside of data.model.src as passed to the template? Doesn't this contain the videopress_guid? This src would be persisted in the widget instance data via the url property and so it could be parsed out of the URL whenever it is needed. So that's why I was suggesting we do a pattern match on the URL.

Otherwise, I'm not a huge fan of hacking the template like this. I would prefer this to either be a first-rate condition OR for there to be some sort of filter that can be used to decide how this works.

Yeah, it's not great.

The obvious first rate condition would be for the template to have a data.iframe_src that could be provided to it just the way data.is_hosted_embed is provided. This would provide for more ways in the future to also add other embeddable media sources here.

Yeah, so essentially we need to make the is_hosted_embed extensible instead of being limited to just YouTube and Vimeo?

At the moment, when is_hosted_video then it will attempt to fetch the thumbnail image for the video via oEmbed. It doesn't currently embed the actual video player. Nevertheless, I think we could actually switch back to embedding the oEmbed response because we originally opt-ed for the static image for the sake of… I think it was the oEmbed getting re-rendered with each change, is that right @timmyc?

@dbtlr So then what about on the frontend side of things. How is VideoPress working with the video shortcode? Is it hooking into wp_video_shortcode_override to override the output when a VideoPress player? Or is the MP4 still used natively by VideoPress and that the iframe is only used during upload?

dbtlr commented 7 years ago

@westonruter The VideoPress shortcode is handled here: https://github.com/Automattic/jetpack/blob/master/modules/videopress/shortcode.php and it definitely uses the wp_video_shortcode_override filter.

That mp4 link isn't useful for anything. In this case it is a compatibility fragment, the player itself is hosted at https://videopress.com/v/$guid and that is added into an iframe. That player handles many events, including letting a user know that the upload is still being processed, which is the main reason to use it here.

I suggested using the videopress_guid field because removing the guid in JavaScript is a PITA. The matching regex looks something like this though /^https:\/\/videos\.files\.wordpress\.com\/([a-zA-Z0-9]{8})\/ the replace that was being used in the code sample above does not account for the guid vs filename. In this case, the filename is basically junk, all you case about is the guid.

westonruter commented 7 years ago

The attributes for the video widget are more fixed than the attributes for the shortcode. In any case, I think the url attribute is what we want because essentially even though the video was uploaded to the media library, it is in actuality being used as an oEmbed.

Here's what I'm thinking we do.

In core the changes we can make are:

  1. We export the oEmbed providers defined in \WP_oEmbed::$providers to JavaScript so that we have all of the URL patterns for recognized oEmbeds, including VideoPress.
  2. When someone doesn't provide a URL that ends in a recognized file extension, we check the list of oEmbeds to see if it is recognized.
  3. The control's preview template when it sees the url is not to a file can embed the oEmbed html response, where currently we are using this response to embed the poster image. (I suppose we'd have to write this HTML into a blank iframe to account for the JS it would load, and we'd set the width and height of the iframe according to the oEmbed response. Special care would be needed here when moving the widget to a new location, as the iframe would get destroyed and it would have to be re-rendered.)

Then, all that the VideoPress plugin needs to do is make sure that when a user selects a VideoPress video, that we rewrite the file URL to be the oEmbed URL instead. And this could be done via enqueueing the following JS that extends media-video-widget:

wp.mediaWidgets.controlConstructors.media_video.prototype.mapMediaToModelProps = (function( originalMapMediaToModelProps ) {
    return function( mediaFrameProps ) {
        var matches, props = mediaFrameProps;
        matches = props.url.match( /^https?:\/\/videos\.files\.wordpress\.com\/([a-zA-Z0-9]{8})/ );
        if ( matches ) {
            props = _.extend( {}, props, {
                url: 'https://videopress.com/v/' + matches[1],
                mp4: '' // @todo Would there be other file types returned?
            });
        }
        return originalMapMediaToModelProps.call( this, props );
    };
}( wp.mediaWidgets.controlConstructors.media_video.prototype.mapMediaToModelProps ));

So that would make it so that the url populating into the widget would be the actual oEmbed URL as opposed to the file URL.

Thoughts?

timmyc commented 7 years ago

I think it was the oEmbed getting re-rendered with each change

We fixed the re-rendering of the preview on title changes so that isn't much of a concern anymore - I seem to recall the final reason for not showing the oembed frame was due to styling/space constraints but I would need to go back and read some old PRs to verify this. Using the oembed response is totally doable, like you said as long as we manage the re-constructing of iframes on move.

I was hoping to try and work on a fix for this today, but other items have so far distracted my day. If the VideoPress GUID is available in the selection, or someway accessible in the mediaframe models - we could potentially try to utilize it and the special mime type to figure something out.

In the same flow in the editor, things are handled by utilizing the videopress shortcode - is that worth exploring here at all? We would still have an issue with the widget preview even if that worked for the front-end render.

westonruter commented 7 years ago

I was hoping to try and work on a fix for this today, but other items have so far distracted my day. If the VideoPress GUID is available in the selection, or someway accessible in the mediaframe models - we could potentially try to utilize it and the special mime type to figure something out.

Can't we do this by looking at the url as returned from the media select frame as in https://github.com/xwp/wp-core-media-widgets/issues/160#issuecomment-301678111? Or I suppose the media select frame actually would return the bare videopress_guid so it could actually be used to construct the proper oEmbed url when mapping the media props to the model props.

In the same flow in the editor, things are handled by utilizing the videopress shortcode - is that worth exploring here at all? We would still have an issue with the widget preview even if that worked for the front-end render.

Utilizing the videopress shortcode would mean we'd have to do an ad hoc Ajax request to render. I think it would be better to keep it generic to either be an oEmbed url or a file url.

timmyc commented 7 years ago

Just looking at XHR traffic during the upload process from the media modal on a videopress enabled site.

First request is to admin_ajax, Headers:

POST
filename:fontinalis.mp4
action:videopress-get-upload-token

Response

{"success":true,"data":{"upload_token":"redacted","upload_blog_id":71257737,"upload_action_url":"https:\/\/public-api.wordpress.com\/rest\/v1.1\/sites\/71257737\/media\/new"}}

The second request is an OPTIONS request to the upload_action_url for the CORS request to follow.

The final XHR, Headers

POST
:authority:public-api.wordpress.com
:method:POST
:path:/rest/v1.1/sites/71257737/media/new
:scheme:https
accept:*/*
accept-encoding:gzip, deflate, br
accept-language:en-US,en;q=0.8
authorization:X_UPLOAD_TOKEN token="redacted" blog_id="71257737"
cache-control:no-cache
content-length:1140233
content-type:multipart/form-data; boundary=----WebKitFormBoundarySgTchnMjzx8VQY3O
origin:http://haiku2.com
pragma:no-cache
referer:http://haiku2.com/wp-admin/customize.php?return=%2Fwp-admin%2F
user-agent:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Response

{"media":[
    {
        "ID":337,
        "URL":"https:\/\/haiku2.com\/fontinalis1-mp4\/",
        "guid":"http:\/\/haiku2.com\/fontinalis1-mp4\/",
        "date":"2017-05-17T00:45:17+00:00",
        "post_ID":0,
        "author_ID":0,
        "file":"fontinalis1-mp4",
        "mime_type":"video\/\videopress",
        "extension":null,
        "title":"fontinalis1-mp4",
        "caption":"",
        "description":"",
        "alt":"",
        "icon":"https:\/\/wordpress.com\/wp-content\/mu-plugins\/videopress\/images\/media-video-processing-icon.png",
        "thumbnails":{},
        "meta":{
            "links":{
                "self":"https:\/\/public-api.wordpress.com\/rest\/v1.1\/sites\/71257737\/media\/0",
                "help":"https:\/\/public-api.wordpress.com\/rest\/v1.1\/sites\/71257737\/media\/0\/help",
                "site":"https:\/\/public-api.wordpress.com\/rest\/v1.1\/sites\/71257737"
            }
        }
    }
]}

The final URL for the above video is https://videos.files.wordpress.com/PyeqpaUs/fontinalis1_hd.mp4 - so the GUID needed to build the appropriate embed url of https://videopress.com/v/PyeqpaUs is not available in the initial media upload payload that I can see.

dbtlr commented 7 years ago

@timmyc I've fixed the API on the WordPress.com servers to correctly return the videopress_guid field. This is the first time we've needed it in this context, so it was missing by mistake.

timmyc commented 7 years ago

@dbtlr fantastic thanks for doing that! Hopeful we can get a fix now with that attribute in the payload.

timmyc commented 7 years ago

@dbtlr looks like the videopress_guid is always returning null currently, I'll send you a diff that fixed the problem on my sandbox to see if it looks good to you.

timmyc commented 7 years ago

Closing this ticket out and tracking at https://core.trac.wordpress.org/ticket/40808 since the video widget is now in core.