kmadel [kmadel]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

I wonder how HTML5 output can be mixed with Drupal output, which is XHTML 1.0 transitional by default. Did you check if the output of the module passes the W3C validation?

The problem is that some tags defined in HTML 5 are not defined in XHTML; defining a document as XHTML, and adding HTML 5 content is like defining a document HTML 4, and adding XHTML content.

Just another thought in the web standard discussion I see here. Drupal is not only used in the web. Drupal can be and is used in an intranet situation where the organisation determines the standard (which could be one family of browsers). In that situation the HTML5 and XHTML mix becomes less important and the module proposed here is then fully justified when that same organisation for their own reasons (may be security) rejects closed source standards like Flash.

I strongly support the acceptance of such a module. @kiamlaluno's concerns about video tags in the XHTML Doctype are probably misplaced, as HTML5 has an accompanying XHTML5 spec that should allow Drupal's HTML to validate. (However, the doctype would need to be changed from XHTML transitional.)

Trust the Drupal users to make decisions about how to structure their sites - preventing important functionality because it might lead to HTML that doesn't validate to the DTD (which will only trigger quirks mode, after all) doesn't seem like a good decision.

While @kiamlaluno has a point about the usefulness of standards I strongly agree with @kmadel and @michaek that there are times when we are at crossroads and such decisions should be left to the discretion of the individual.

We're having the video element thrust on us prematurely by the shear number of users. The fact that the element itself is not entirely standards compliant is hardly the fault of the module's author and is not a compelling argument for the exclusion of this module.

As @kmadel noted there are already approved modules online that are leveraging the "video" tag. He mentioned "Mediafront" and I am also aware of the module "Video for Everybody!".

Let me also add that I have tried @kmadel's "Html5 video" module as well as the two others mentioned above and I've found his approach to be unique and would be a valuable addition to the module repository. I'd certainly use it!

So +1 for giving the gentleman his CVS account so we can all benefit from this module.

this module/concept should be the singular focus of media for drupal--- and its a shame it is not live! please approve and get current!!!!!

There is no need to wait for Drupal 8 to support HTML 5 and the video tag. Any version of Drupal can output HTML5 just by changing the doctype. In fact, if a theme template already supports XHTML transitional, by just changing the doctype it will convert the whole template to HTML5 (the XHTML variant).

So there is no need to hold on such module because it wants to use HTML5 tags.

That being said, here are one suggestion and one issue.

Suggestion: In the readme file, add information on how to modify the doctype of a theme template file in order to have an HTML5-compliant doctype.

Issue: Remove the flowplayer player completely. This player to not use GPL2 licencing so the player cannot be hosted on d.o.

As my previous comment. :-)

Suggestion- instead of using Flowplayer, take a look at http://videojs.com/ (LGPL License, WebM support) or http://flarevideo.com/ (MIT license). Since neither is GPL, you sill couldn't host the video player on d.o., however, you could allow the user to upload the player to the libraries folder, the way WYSIWIG and other modules handle it. Personally, I like VideoJS because it supports WebM.

I hope you get a CVS account soon! A Drupal 7 version would be sweet, too :)

@kmadel, you do need to remove the flowplayer code. 3rd party code is not allowed in Drupal CVS. The standard practice is to include a link in the README.txt to where someone can download the 3rd party code, along with instructions of where to put the code in your module directory (or using Libraries API).

You should also remove the LICENSE.txt, as that gets added automatically by the packaging system on Drupal.org. Everything in Drupal CVS is GPL, so no need to state that in individual projects or files.

Beyond that, I see some minor issues. Markup should be running through theme functions (or templates), for example, in case people want to change it. But that can be done after the project is up.

The HTML5 question is a non-issue now, settled in other threads since that comment. That's not the part that needs work.

Indeed, all codes on d.o. must be GPL2. GPL3 is not allowed. This is why you need to remove Flowplayer.

To complete the information, GPL 3 is not compatible with GPL 2.
Even in the case it would be compatible, code available from third-party sites should not be committed in Drupal.org; Drupal.org should remain the repository for Drupal modules, and themes (and installation profiles).

A few things:

1. In html5_video_format_field(), "/" is repeatedly used in place of base_path().
2. The implementation of "Video for Everybody" is not the same as the current VfE version: each video is in a separate element on VfE, and this implementation uses src on the video tag, which doesn't seem to work for my Firefox (3.6.3).
3. VfE does not make use of browser detection, which is used heavily in this module. Wouldn't it be better without?
4. I'm not enthusiastic about looping over the uploaded files to find the various formats - I know this is a simple way to reuse the file CCK field type, but it doesn't seem very clean to me, and it gets in the way of the convention of setting "Unlimited" to allow multiple videos (in this case, it becomes different formats of the same video).

This isn't intended to count against the acceptance of the module, but to point out necessary (and less immediately necessary) improvements.

I've attached a patch that should address 1 & 2.

In response to @ensignavenger's request not to use Flowplayer, http://videojs.com/ actually does fall back to Flowplayer. http://flarevideo.com/ seems to have its own simple Flash video player - I haven't looked all that closely at it, but it's likely that a more widely used Flash fallback is a better bet. (Also, Flare Video is implemented via Javascript, which doesn't seem necessary.) There's an existing Flowplayer API module for Drupal, as well, to avoid duplicating code: http://drupal.org/project/flowplayer

Flarevideo uses Javascript, but it does so by leveraging jQuery, which should mean that integration with Drupal is light weight.

These are early days for HTML5 players, I think it's reasonable to say we should expect the options available to evolve dramatically over the next few months. I think the idea behind this module is sound, especially if it's relatively easy for anyone interested to add integration for new players as they gain momentum.

I'd also suggest that it makes most sense for the code that specifically supports flowplayer to be moved out to a separate file, and the code abstracted so to facilitate adding support for new players.

I do agree, as well, that it makes most sense for player code to be put into the libraries directory. You can look at SWF Tools or WYSIWYG API as examples of modules that support multiple external libraries.

I was bringing up the Javascript for Flare because it depends on Javascript (that is, it's invoked via jQuery) and some users may have Flash enabled but Javascript disabled. Better one dependency than two.

Will be a useful module + easy to use.
What about to merge with the video_for_everybody module, still in dev: http://drupal.org/project/video_for_everybody

FYI: Couldn't make it work on my FF3.6 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fr; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6), and fallback on FF3 didn't work neither.
webkit Chrome4 Safari4 Mac + PC= fine

Jean

I wasn't aware of, and haven't tested, the http://drupal.org/project/video_for_everybody module, but this seems like it's a pretty clear duplication of those efforts.

This is actually quite a bit different than the video_for_everbody module, and fixes a few ugly quirks that the video for everybody solution has to work with - for example no poster image for the video element because of a nasty iPhone bug.

The fix you applied for your module could be applied also for the existing one.

But the real difference, again, is that my module implements a CCK field type, whereas the video_for_everbody only implements a content type.

That is the difference that really matters. I don't think that opening a feature request to change Video for Everybody! to implement a CCK type instead of a content type is the path to follow.

@kmadel: Thank you for your reply.

I forgot to change status.

1. As already reported, files available from third-party sites, and files that are not under the same license used by Drupal are not allowed to be committed in Drupal.org repositories.
2. The license file cannot be committed in Drupal.org repositories, as the license of everything committed in CVS is automatically the same used by Drupal core code.
   Any reference to the license used for the code should be removed by comments; GNU Public License is too generic.
   The copyright statement would not be anymore valid in the moment you apply patches provided by other users.
3. The presence of an access callback that simply returns TRUE makes me think the code is not complete. As reported in the requirements, the proposed module should be complete.

   /**
    * html5_video menu access check.
    */
   function _html5_video_colorbox_page_access($node_id) {
     //$node = node_load($node_id);
     return TRUE;
   }
   

1. In html5_video_format_field(), "/" is repeatedly used in place of base_path().

What michaek reports is correct.
base_path() should be used any times a module is returning a file path; in the case Drupal is installed in a subdirectory of the web server root directory, base_path() returns the correct base path. There are Drupal functions that return the path of files, and also these functions use base_path().

I think I found what make the module fail on FF. If you have Flashblock active, the fall-back doesn't work witch is the correct behaviour. But where it gets annoying is Flashblock is also hidded to the end user, whose therefore not aware any more is using Flash instead on the native video tag.
So what the module might need, is to detect that Flachblock is activated and in order to let the user see the Flashblock icone. We could do it with jquery, wouldn't it be better if it could be embeded in the module?

Note: I've noticed this behaviour playing with the mediafront modulehttp://www.mediafront.org/. Also I've haven't tested this behavior on the html5_video module, but by deduction, it seems a logical path.

Temporary solution: Detection with Jquery or deactivate Flashblock or provide the ogg file

Jean

Though some of the comments are, this issue as a whole isn't really about "The reason why Firefox fails to play." It's still about kmadel's CVS application, so I'm changing the title back to reflect that.

So I've tested FF without Flashblock. Unlike mediafront, desactivating Flashblock doesn't slove the issue. Neither mp4 nor ogg/ogv plays.
Jean

I've spent a few hours today on the existing video_for_everybody module (#863636) and I'm not convinced this proposed module provides any additional functionality. In fact, I think the vfe approach is superior: it uses a content type with fields for each video format, while this module uses a single CCK field for all formats.

The content type approach can associate any number of videos with a node (using Node Reference), while the field approach can have only one video per node. Further, the approach of specifying different formats of a single video within the same field seems likely to encourage user error. Clearly labeled fields for mp4 and ogv make it simpler for a user to understand how to add video content.

Lastly, using browscap is counter to the philosophy of the video for everybody approach. The goal of the approach is to provide cross-browser video without relying on javascript or user agent sniffing. Certainly, we have the technical capability to abandon that philosophy, but we do so at the cost of elegance and ease of maintenance.

This is a valuable effort, but I think there's every reason to focus energies on the existing module.

Thanks Michael for sharing your video_for_everybody tests.
Is vfe module already stable or you've found major bugs?

Jean

We should discuss this somewhere other than @kmadel's CVS account issue. :) Go ahead and open an issue on the VfE project, and I'll be happy to share my experiences.

michaek --> http://drupal.org/node/865642

Remember to change status, when you upload new code.

1.     'title' => t('HTML5 Video Configuration'),
       'description' => t('Global configuration for the HTML5 Video module.'),
   

   Menu callback descriptions, and titles are not passed to t() as that is already done from Drupal core code.

2. The implementation of hook_uninstall() doesn't remove the Drupal variables defined, and used from the module.
3. It's not clear to me the purpose of the file cache.inc file, and why the code contained on the that file should prefix the value of $cid with a value that depends on the browser being used. There are values in the cache that don't depend from the browser used to access a Drupal site. The modified code doesn't consider the case the module is installed on an already active site; in such case, modules would not get the data saved in cache before the file cache.inc has been replaced with the copy of the file given with the module.
4. The code is using functions from imagecache.module without to verify if the module is enabled, and without to make explicit its dependency from that module.
5. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

For what I am concerned, the code contained in cache.inc has some problems:

• It changes the cache ID for every table (excluding cache_browscap). That means it is going to change the cache ID even for modules that use the cache for data that is not related to content. In example:
  • Apache Solr Search Integration uses a cache for statistics data, and those data don't change depending from the browser used to access the site.
  • The input format output for a node is cached. The output of an input format doesn't generally depends from the browser used to access the site; that is the case for the input formats defined from Drupal core code like full HTML, and filtered HTML.
  • The update manager keeps in cache the information about the most recent project versions; also in this case, the data don't depend from the browser.
• As consequence of what reported from the previous point, there are many modules that keep to rebuild their cache for each of the different browsers (or devices) the proposed module supports, even when that is not necessary. The effect is to make vane the use of cache, and that is even worse than not having a cache at all.
• The purpose of implementing a different version of cache_get(), cache_set() is to use a different way to save the cache data; there are implementations of those functions that use APC, or memcached.

In short time, it's probably easier to change the code in cache.inc to apply the prefix only to some cache tables, or to some cache IDs. So far, Drupal core code uses only the cache table, not other tables.
I still don't get the reason why the module doesn't clear the cache it needs to clear, instead of changing all the cache IDs.

Talking of http://drupal.org/project/browser_theme, the maintainer of that project applied to be co-maintainer of an existing module (see #790604: mark.carver [markcarver]); in such case, there has not been any review of code, nor someone that isn't a good reviewer.

Thanks for your work on this module.

I think your approach of having a separate video CCK field type is significantly and importantly different from the vfe approach.

I just posted a feature request to the vfe issue tracker:

http://drupal.org/node/890218

I suggest that the code used to generate the html 5 code (and deal with all the fall back configuration) be abstracted into an api module. That way both projects could share the same duplicate code and each one could focus on the different approaches to Drupal integration (for what it's worth - I'd like to see a module that implements an input filter that would convert [video src="/path/to/video"] with the proper html5 code - which is yet another implementation of the same idea).

Hope that's helpful.

jamie

So what happened here? Did Kmadel get a CVS account?

That demo he showed on http://www.captechconsulting.com looked fantastic. It's one of the few (only?) examples of HTML5 video working in Drupal.

The Maintenance status of Video For Everybody is currently "Unknown." We REALLY need this module and the development effort that has gone into it.

I understand there were a few outstanding issues. I got nervous about changing the cache ID for all tables. But we had a person that had done some exceptional work and I would be willing to test this module today and support the effort to bring it to Drupal 7.

Just saying I hope we didn't run Kmadel off.

I agree with @mcfilms on this. I favor v4e philosophically, but I don't think there's any reason not to have multiple approaches. (Actually, I think the process of being approved for CVS accounts is pretty anachronistic.)

I've done some further work on the v4e module on my own, some of which I've posted as patches in issues, but I'd be happy to put the rest out there for the community. Probably on github. :)

The module may be great, but this is an application for a specific person to gain CVS access. That person disappeared from the thread, so the application died. That's not a rejection of the module.

Oh, I completely understand. But it seems somewhat distorting to community involvement that one's access to CVS is often based on the acceptability of a specific module (and definitely was in this case). @kmadel was clearly doing a lot of good work on his module, but the discussion around accepting his CVS application (and the ultimate closing of the issue) was concerned with whether his module would duplicate the functionality of other modules.

Actually, I may be mistaken about why the issue was closed - I don't want to assume that I know why @kmadel left this discussion, or why @kiamlaluno closed the issue.

sreynen is correct on comment #58: The user who applied for a CVS account didn't change the code as reported by the review, nor did attach a new version of the module after comment #51; I take the user lost interest.

Well my point of contention is that Kmadel responded to all the issues as they arose. When questioned about his module duplicating another module's work, he explained how and why it was different. There were some suggestions on making some more improvements to the module, but should these have prevented him from getting a CVS account?

I'm just curious what the criteria is and at what point somebody says "awww heck, you know what? The benefit of what you are doing far outweigh the potential hazards."

Please look at comment #85 on this very long thread about CVS here:
http://drupal.org/node/703116#comment-3122264
I think this person's idea is better than the current method of CVS account review.

I wrote about this issue once before here:
http://drupal.org/node/811402

I reported in #51 what is wrong with the code, but kmandel didn't change the code.

@mcfilms, I completely agree it's important to question how we can improve this process, but this particular thread isn't the place to have that discussion.

Not only is it a place where no one else will find it, but it's not really relevant here. It's simply impossible to say to @kmadel "the benefit of what you are doing..." because @kmadel is no longer here and no longer doing anything we can see. As far as I can tell, @kmadel has been absent from Drupal.org in general recently, not just this thread. Unless you're arguing that participating in your own application thread shouldn't be a prerequisite to having a CVS account, all other considerations are made irrelevant by @kmadel dropping out of this thread.

I am happy to have this discussion elsewhere. Is there an ongoing thread I can join?

I bring this up every so often because I know there is at least one moderator listening and hope that I can effect incremental change. (Also, I thought the work he was doing was valuable and maybe I could woo him back.)

I think #703116: Our CVS account application requirements are obtuse and discourage contributions, which you pointed to, is where this conversation is happening. The issue is currently postponed because several aspects of the process will necessarily change with the move from CVS to git, but that needn't stop the discussion.

Welcome back! I am very interested. Currently I am working on D7 and HTML5 with video. I have some degree of success with the video.js player hardcoded into my pages. I'm also testing the Mediafront module for D7. But neither solution is perfect.

@kmadel, you also might consider offering to team up with Travist and see if you can work together on the MediaFront module. Maybe you can prune some of the javascript weight and make it easier to play video in iDevices.

I'm also still really interested in this, and I've done a certain amount of work with the video_for_everybody module. Count me in as willing to contribute to a solid HTML5 video module.

I too am still interested. Good to see you back!

Would really like to see a D7 version of the module

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

I'm really glad you're back! Subscribing.
-kelly

Very interested in this module as well.. I think it is much needed.

As per previous comment, I am setting this issue as Won't fix.
Since new users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.