craft-embedded-assets icon indicating copy to clipboard operation
craft-embedded-assets copied to clipboard

Include video durations?

Open bstein-clever opened this issue 7 years ago • 8 comments

I'm a huge fan of your neo plugin, and just discovered this one, which looks just like something we were needing for youtube videos.

How hard would it be to add a duration value for the cached video data? I'm assuming (but haven't tested) that it would be possible to update cache info by doing a cache update, but doing so for an individual entry would be pretty cool.

One extra wrinkle: our site is still on Craft 2 (we use neo heavily), so I know we're not on the lastest version of the plugin.

bstein-clever avatar Jul 10 '18 20:07 bstein-clever

+1 for retrieving video duration

beneklisefski avatar Jun 05 '19 23:06 beneklisefski

+1

johndwells avatar Oct 15 '20 20:10 johndwells

@ttempleton Would you be interested in a PR for this? I could definitely add support for Vimeo and Youtube. Vimeo returns duration in the oembed response so it would be easy to grab. Youtube's duration is deep within the html provider response, so that could be scraped out. I'm not familiar with other video platforms though (pbs video for example).

johndwells avatar Oct 16 '20 13:10 johndwells

@johndwells We only really support YouTube and Vimeo for the other video-specific embedded asset methods, so it'd be fine if video durations only supported those too. But if you'd like to work on a PR for adding video durations, then we'd definitely merge that.

ttempleton avatar Oct 19 '20 07:10 ttempleton

I'm reconsidering submitting a PR for this, and instead submitting a separate PR to add in some event hooks so that things like this (and others) can be done instead.

My main thinking is that the underlying library, Embed, does not provide an interface for accessing vimeo duration, and I think that you'd probably want to stay within the lines of what Embed provides (doing otherwise may prove a slippery slope!). But also, I think that Embedded Assets could benefit more, sooner, from having some simple event hooks in place that let others extend as necessary for such unique circumstances as this.

  • J

On Mon, Oct 19, 2020 at 2:02 AM Thomas Templeton [email protected] wrote:

@johndwells https://github.com/johndwells We only really support YouTube and Vimeo for the other video-specific embedded asset methods, so it'd be fine if video durations only supported those too. But if you'd like to work on a PR for adding video durations, then we'd definitely merge that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spicywebau/craft-embedded-assets/issues/74#issuecomment-711709727, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABAXXSQAVPGYFDWJDUZRVDSLPP7XANCNFSM4FJIFPHA .

-- ▂ ONE DARNLEY ROAD

John D Wells Technical Director

1C Darnley Road, London E9 6QH +44 (0) 208 5259 292

http://onedarnleyroad.com http://facebook.com/onedarnleyroad http://twitter.com/onedarnleyroad

johndwells avatar Oct 19 '20 13:10 johndwells

I don't necessarily have a problem with adding functionality that isn't directly provided by Embed. Having thought about it a bit more, though, I do agree with the slippery slope comment in that we shouldn't keep adding embedded asset functionality that's only relevant for certain types of embedded assets or for certain providers. If there were event hooks to allow extending to include video durations or other video-specific information, that seems like a good solution to that situation.

ttempleton avatar Oct 26 '20 07:10 ttempleton

@ttempleton I've had a play with creating an event which allows me to append arbitrary additional information (in this case, duration) to the array returned by the adapter. But then encountered 2 hurdles:

  1. Any attributes not supported on the EmbeddedAsset model causes the entire model to not be instantiated (https://github.com/spicywebau/craft-embedded-assets/blob/master/src/Service.php#L223-L225).

  2. What is stored on the filesystem is a serialized json object representing the EmbeddedAsset model, not the array created from the adapter response and used to instantiate the model.

This means that even if #1 is addressed to quietly discard/skip any non-supported properties, the custom array values are entirely lost once the EmbeddedAsset is serialized and stored.

I can think of one way to work around this, but it would be significant: allow plugins to provide their own EmbeddedAsset model. This would entail making an EmbeddedAssetInterface, allowing plugins to register their own, etc... quite convoluted, possibly riddled with pitfalls, though it would be the most flexible down the road.

A simpler solution might be to introduce an "extras" attribute on the EmbeddedAsset model and allow that to hold arbitrary information that may be added by 3rd party plugins.

Interested to hear your thoughts. To be honest, I may end up having to bail on this and resort fo my own oembed fetches, since I'm doing far more with the duration than merely displaying it. So even getting Embedded Assets to store the duration only solves one part of my puzzle. So what I suppose I'm saying is, if you feel like this ticket is just generally going down a road you don't want to travel, that's OK. I'll find another route to solving my particular issues.

Thanks!

johndwells avatar Oct 28 '20 14:10 johndwells

@johndwells I think the extras attribute is an interesting idea and would be a good solution that we could implement in the shorter term, I'll have a look at that in the next couple of weeks.

I do like the idea of providing an interface, and in addition to allowing plugins to provide their own models for their own requirements, I could see us then providing an Embedded Video model which could have the specialist functionality like video ID/duration/etc. I definitely want to work on the extras attribute first, though, but it's an idea that I'd like to try out in the future.

ttempleton avatar Nov 01 '20 15:11 ttempleton