Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aaron’s picture

Project: Embedded Media Field » Media: YouTube
Version: 6.x-2.x-dev » 6.x-1.x-dev
Component: Providers » Code

yes, but not in emfield; this kind of feature quest would go to media youtube now. emfield v2 has no providers, and v1 is only bug fixes.

xjm’s picture

Ah, sorry, I forgot that the youtube plugin was actually a separate module. Thanks.

Danny Englander’s picture

I'm interested in this also, subscribing.

patrickroma’s picture

Subscribe

effulgentsia’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Active » Needs review
FileSize
23.91 KB

I know this is eagerly wanted in the D6 version, but since it's a feature request, I'm gonna hijack the thread and re-assign it to D7. Then, once it's in D7, it can be backported to D6.

Here's the code we're using on DrupalGardens. It's working. Some of it is still a little ugly. Not sure how much we want to perfect it before committing it, vs. committing it somewhat raw, and using follow-up issues to improve it.

Note, there's an existing unused js/media_youtube.js file in the branch. This moves that to js/media_youtube.old.js, and puts a new JS file into js/media_youtube.js. But because CVS is stupid, this shows up as a bunch of -/+ signs in the patch file, so probably easier to review by applying and viewing in context.

effulgentsia’s picture

+++ media_youtube.module	22 Oct 2010 21:20:30 -0000
@@ -244,31 +262,93 @@ function theme_media_youtube_embed($vari
     if ($thumbnail) {
-      $output = theme('image', array(
+      // @todo Clean this up.
+      $image_variables = array(
         'path' => 'http://img.youtube.com/vi/'. check_plain($parts['v']) .'/0.jpg',
         'alt' => $variables['alt'],
         'title' => $variables['title'],
-        'attributes' => array('width' => $width, 'height' => $height),
         'getsize' => FALSE,
-      ));
+      );
+      if (isset($preset['image_style'])) {
+        $local_path = 'public://media-youtube/' . check_plain($parts['v']) . '.jpg';
+        if (!file_exists($local_path)) {
+          $dirname = drupal_dirname($local_path);
+          file_prepare_directory($dirname, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
+          @copy($image_variables['path'], $local_path);
+        }
+        $image_variables['path'] = $local_path;
+        $image_variables['style_name'] = $preset['image_style'];
+        $output = theme('image_style', $image_variables);
+      }
+      else {
+        // We need to add this style attribute here so that it doesn't get lost
+        // If you resize a video in a node, save it, edit it, but don't adjust
+        // the sizing of the video while editing, the size will revert to the
+        // default.  Adding the specific size here retains the original resizing
+        $WYSIWYG = isset($variables['object']->override['style']) ? $variables['object']->override['style'] : '';
+        $image_variables['attributes'] = array('width' => $width, 'height' => $height, 'style' => $WYSIWYG);
+        $output = theme('image', $image_variables);
+      }

Full disclosure: the above code has nothing to do with html5 support. It actually implements a couple separate issues related to thumbnail images and WYSIWYG. But I'm too lazy to pull that out of the patch file at the moment (oh, CVS, and your amazingly byzantine quirks). If this issue goes through a long review process, I can pull it out during a re-roll. Or, if the issue is RTBC'd quickly, I can pull it out when committing. It has no affect on being able to test the patch.

Powered by Dreditor.

aaron’s picture

Status: Needs review » Reviewed & tested by the community

Uggh. That theme function is getting really funky. We really need to pull that into a template file and properly preprocess it. In addition to the inline js that needs to be pulled out.

However, it does work as advertised, and all that can be addressed in another ticket once we get this in there.

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed to CVS minus the hunk in #6. There's already @todo lines included for replacing the inline JS with proper D7 behaviors. But yeah, replacing the theme function with a template is also a good idea.

effulgentsia’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)

Fixed for D7 branch, but not yet for D6 branch. Moving this issue back to 6.x queue.

aaron’s picture

Status: Patch (to be ported) » Needs work

needs a whole lotta loving for d6, which has a really too large theme function already...

Michsk’s picture

subscribing, just bought jwplayer 5.3 for the html5 option and now am reading that it is not yet implemented ;)

arsenalpilgrim’s picture

This is a rather critical problem for those of us who want one solution for video irrespective of device. I was hoping to use media youtube to embed the html5 (iframe) code and it would work from a iphone or desktop/laptop without further changes. Sounds like I will have to go another path of a separate mobile site just for the videos.

Or if there is another option please let me know.

I'll take a look at a few other media players.

patrickroma’s picture

try vimeo... that should work!

arsenalpilgrim’s picture

vimeo has its own player for HTML5 with potentially its own set of issues. Ideally I want to have a player (such MediaFront) that sits ontop. It relies upon EMbedded Media Field. I think I will add more complexity to my environment if I have different players for youtube, vimeo, etc.

I'm not sure if Embedded Media Field VIMEO mod supports the new html5 player from vimeo. I guess I will try it and see.

Did you already test vimeo?

UPDATE: December 9th.
Used Vimeo as the embed instead of Youtube and video plays on iPhone. Note I did need to change an option on Vimeo account to enable mobile devices. So, Vimeo will be a workaround for now, but would like youtube support. Not able to get the MediaFront viewer to run in the iPhone, a separate issue. Also, for other users, after changing your vimeo setting to allow mobile device support, it might take several minutes for your videos to be updated.

JohnTarr’s picture

Subscribing for D6 support

bcobin’s picture

Subscribe

zet’s picture

Subscribe

Garrett Albright’s picture

Pizzatch attizzached. I thought this would be an easy fix, but it seems that either YouTube has changed other things about the data they return via the API, or this module had weird workarounds and such that were breaking - probably a bit of both. Either way, after applying this patch, using both old- and new-style YouTube embed codes works.

aries’s picture

I've also made a HTML (iframe) patch for 6.x-1.2. It's sponsored by KOBA, thanks!

What currently works:

There's a HTML5 player option under admin/settings/media_youtube in "YouTube video player options" fieldset.
If you checked it,then the HTM5 iframe version will be used. Otherwise, the old, flash based output will be used.
The relevant general settings (width, height, fullscreen) are in use as previously with the flash method.

Please let me know if it works or not for you.

effulgentsia’s picture

Status: Needs work » Needs review

Thanks Garrett and aries. I'm assuming #18 and #19 are intended for review, so setting issue status accordingly. Set back to "needs work" if I'm mistaken. I haven't been involved at all in the D6 version of this module, so I leave this to aaron to decide on, though anyone who wants to offer a constructive review is welcome to, and doing so will likely help aaron.

Garrett Albright’s picture

effulgentsia, yes, sorry about that - forgot to change the status.

To those who may be unsure, aries' and my patch are completely different in what they do. Mine will make it so that newer-style embed codes can be parsed by the module so the the video ID can be properly extracted, whereas aries' appears to allow the module to display videos using YouTube's HTML5 player. You should be able to use either patch or both depending on what functionality you need.

aries, for future reference, please don't compress patches. Uncompressed patches can be viewed straight in one's browser without having to download and decompress the file - a bit of a pain if you just want to quickly peek at the patch and see what it changes. In addition, for Drupal 7 patches, the automatic testing bots won't work properly with compressed patches.

aries’s picture

Sorry Garrett, I forgot the compression, in the early Drupal dev time, this was a standard and sometimes forgot that this has been changed.

bcobin’s picture

Version: 6.x-1.x-dev » 6.x-1.2

Applied both patches (#18 and #19) cleanly - I get the following error message (last part only):

warning: include() [function.include]: Failed opening '.sites/all/modules/media_youtube/themes/media-youtube-html5.tpl.php' for inclusion (include_path='.:/usr/lib/php:/usr/local/lib/php') in /home/[site_name]/public_html/includes/theme.inc on line 1066.

Changed the version on this thread to 6.x-1.2 because that's the versions the patches are against - looks like it's a matter of correctly finding the .tpl file.

UPDATE: Upon looking at this further, it seems there is no media-youtube-html5.tpl.php file in /themes or anywhere else, for that matter... no surprise it couldn't be found! Yikes! Am I missing something here?

Thank you aries and Garrett here - hopefully this should be a simple fix - thanks much!

YK85’s picture

subscribing

aries’s picture

@bcobin Thanks for your response. I forgot the -N argument when I executed the diff, so the new file(s) wasn't included. Sorry.

bcobin’s picture

Thanks, aries!

Applied the patch and no more error - video plays in FF and Safari. (Haven't tested yet in IE.)

But using the iPhoney, there's just a blank space where the video should be - no error - just blank space.

I'll see if I can check on a real iPhone/iPad, but for now this is a "thank you," as well as a possible "heads up!" (Mainly thanks, though...)

kbasarab’s picture

Aries patch in #25 didn't work for me (Was still getting an invalid URL error when adding the iframe to the node). However the patch by Garrett in #18 did work without any problems for me.

aaron’s picture

Status: Needs review » Fixed

Awesome! Thanks so much, @aries & @Garrett Albright. I've committed the patches at #18 and #25. I also added an update function to rebuild the theme.

nasi’s picture

Status: Fixed » Needs review

I think it's worth pointing out, for clarities sake, what seems to be a slight misconception regarding YouTube's embed codes.

The iframe embed codes are not HTML5 player codes, they are codes that should work on all systems, delivering either an HTML5 video player or a Flash-based player, depending on browser capabilities and user settings.

The old object embed codes with only deliver a Flash-based player, which means it won't work on most mobile devices, etc.

So, use object and only some of your visitors can view your videos, use iframe and everyone can.

Personally, I don't see the point in having an option to select which code to use as iframe serves all use cases and is now the default code supplied by YouTube pages (also, calling it an 'HTML5 player' option is misleading).

If there are good reasons to keep the legacy object code then apologies and please do enlighten me!

(I set the status like this in the absence of a 'needs discussion' setting')

nasi’s picture

Version: 6.x-1.2 » 6.x-1.3
Status: Needs review » Needs work

Just reviewed the 6.1.3 release and there is a bug in the 'HTML5 player' output.

The iframe code is producing a url of the form http://www.youtube.com/v/VIDEO_ID where it should be using a url of the form http://www.youtube.com/embed/VIDEO_ID

This is the reason why people are seeing blanks on their iPads, etc., (reported by #26) as YouTube only serves them correctly when using the /embed/ url.

Changing 'v' to 'embed' in media_youtube_theme.inc fixed the problem.

aries’s picture

I'm not sure the HTML5 option is misleading. I checked an embed url with cURL and Links, it serves the HTML5 by default. I haven't found a browser which receives a different source. (Tried browsers: IE8, Firefox, Konqueror, Opera, Links, cURL… i couldn't check it with Safari versions.) This doesn't mean there isn't any, but the majority of the people will receive HTML5 code.

aries’s picture

Good point on the "embed" part of the path. I'm going to make a new patch. Thanks, Nasi!

nasi’s picture

To quote from the page linked in the OP (my emphasis):

If you use the new embed code style, your viewers will be able to view your embedded video in one of our Flash or HTML5 players, depending on their viewing environment and preferences. Environments that support the HTML5 video player are listed here on our HTML5 settings page. In instances where HTML5 isn't supported (e.g. our HTML5 player can't play videos with ads), we use Flash.

That's pretty unequivocal that this is a multi-purpose embed code.

As for what is delivered, some examples:
On my iMac (with Flash installed) I get a flash player delivered to both Safari and Firefox by default.
On my MacBook Pro (no Flash installed) I get an HTML5 video player delivered to Safari and a 'You need to upgrade Flash' warning in Firefox (as it doesn't support H.264).

The point is, there is no 'default' behaviour. It all depends on the user's setting and preferences (Perhaps if I was logged in to YouTube and set the 'use HTML5' option, then that is what I would get on my iMac).

aries’s picture

Ok, then what is your suggestion for the option names and description fields? IFrame or "New method" won't help the users.

JohnTarr’s picture

Regardless of how you decide to label it, it works great. Thank you for the patch, you made a lot of iOS users very happy.

nasi’s picture

The question on my mind is not so much how best to name the options, it is a more fundamental one of is there any need to continue to support the old (and presumably at some point deprecated) method?

Since the new, iframe method should work for everybody, why complicate matters? Why offer an additional configuration that may appear to many admins as if they have to choose between a flash player and an html player?

So my solution would be to scrap the old embed codes entirely and just use the new iframe codes.

As an added bonus, this has the benefit that every site where the admin hasn't considered non-flash users, and doesn't realise there is an option to enable, will end up with a more accessible site by default. Think how much of a nicer place the web would be then! :)

Michsk’s picture

I haven't read the whole discussion, but an iframe sounds like a strange option to me. Espesially since a lot of us use jwplayer or another 'own' player.

aries’s picture

Currently, it's safe to offer both, because the iframe has an independent DOM, so those who are count on tweeking the object tag miss this "feature". But, enabling the "HTML5" option by default is a good idea.

aries’s picture

Hi, I just noticed that my patch was used in the 6.x-1.3 . Great, thanks! Here's the last(?) fix about the "embed" version of the url.

queenvictoria’s picture

Thanks everybody for the work so far. I've added a patch to 6.x-1.3 **after** patching with @aries work in #39 to support the autoplay switch. Attached.

YK85’s picture

I'm unable to see youtube videos when viewing my site on my ipad and iphone. On the PC is works fine.
Does this feature request have something to do with this issue? Thank you!

aaron’s picture

Version: 6.x-1.3 » 6.x-1.x-dev
Status: Needs work » Fixed

yay, great work everyone!

i fixed up the url, allow the query to be overridden easily when calling the theme manually, and default to html5 iframe videos now.

committed!

and yes, @YK85, this issue has everything to do with your issue. update to the latest dev, and it should work fine for you.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

smoothify’s picture

Status: Closed (fixed) » Needs review
FileSize
1.33 KB

I wasn't sure whether this should go in this issue, or whether it should go in a new one but I wanted to link to the previous work that went into the new embed code.

I switched to using the new embed code, but noticed two things :

1) the high quality option has no effect, its always set to high quality,
2) the remove title & info option in the UI also has no effect - the title shows on the video even when selected to be hidden.

Here is a patch that addresses both of these missing settings. The code itself is taken mainly from the regular embed's code.

xpla’s picture

Hi,

there must somewhere a big bug. I can't embed a youtube video using HD as default. Always getting 360p. If i don't use the latest version, the version without html5 support my videos embed as 720p but no playback on iPhone.

If i add the patches or use de DEV Version i could play the videos on my iPhone but it's rendered very strange. Thumbnail jumps around until i start the video. Thumbnail/Embed size isn't also correct on my iPhone.

-> No HD Playback when using latest DEV / Patches
-> Distorted rendering of preview/thumbnail on iPhone

PLS help

jeffnine’s picture

Are these patches going to be added to a new release soon? I'm working on a site where I can't use dev code on a live site (internal policy).

jerry’s picture

Subscribing.

DeadSuperHero’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev

Has this made it into the D7 version at all? I'm trying to ensure that my player uses HTML5 embed by default with Flash as a fallback to ensure that all users have an opportunity to watch videos on my site.

For reference, I'm using MediaFront as a front-end on top of my media source, but I'm sure that the culprit is Media: YouTube as it plays Flash by default no matter what player or display formatter I use for it in my video content type.

Can anyone just walk me through how to set HTML5 embedded videos to default on the D7 version? I'm using the latest dev package of the module, if it helps at all.

aaron’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev

@DeadSuperHero: the d7 version of this module already uses the iframe (which chooses between html5 and flash), falling back to the flash if there's no js. please open another issue for that; this issue is for backporting to d6; the systems between emfield & media are too different.

for my ref; looks like the patch at #44 needs to be reviewed.

thanks!

TimAlsop’s picture

I am also having the same problem as DeadSuperHere, as described in post #48. I find that when using MediaFront module, the YoutTube videos won't play on an iPad device, but they do on Mac, Windows etc. I suspect this is due to the Media_YouTube module using the URL http://youtube.com/watch?v= instead of http://youtube.com/embed/

I am not sure if this is the right thread to discuss this, since in post #5 effulgentsia said that he was going to make this change in D7 and then backport to D6. Now, it seems that patches are being created for the D6 version of this module, instead of D7. So, can somebody advise whether I can expect a patch for D7 or if I need to open another ticket, or maybe there is a known solution to this issue already ?

Thanks,
Tim

orthomeo’s picture

The HTML5 in the dev version works great for single YouTube videos but doesn't work for playlists. I have got it to work by replacing:
$variables['url'] = "http://www.youtube.com/embed/{$variables['video_id']}";

in media_youtube.theme.inc

with:

  if ($variables['item']['data']['playlist']) {
    $video_id = substr($variables['video_id'], 9);
    $variables['url'] = "http://www.youtube.com/p/{$video_id}";
  }
  else {
    $variables['url'] = "http://www.youtube.com/embed/{$variables['video_id']}";
  }

I tried other formats for the playlist url, but only the /p/ format worked. Perhaps someone with a better understanding of how all this works can tweak it for a permanent fix to the module.

Toongenius’s picture

Version: 6.x-1.x-dev » 6.x-1.3
Category: feature » bug

I am using version 6.x-1.3 and I still needed to implement the code change in #30. This has not been fixed in this update as stated. I ticked the HTML5 box in the settings and it made no difference. It wasn't until I did the code change that it worked. I think this is no longer a feature request- it has become a bug.

betovarg’s picture

subscribing

RobW’s picture

@#45, re HD: Some of the options in the current module only work for one or two versions of the player (AS2, AS3, and iframe). There's a good breakdown here: https://developers.google.com/youtube/player_parameters#Parameters.

steinmb’s picture

Version: 6.x-1.3 » 6.x-1.x-dev
Status: Needs review » Needs work

@Toongenius - All patches and work is done against dev.