Detection of file formats / codecs should not be by file extension.
K-Max - February 22, 2009 - 00:41
| Project: | FFmpeg Wrapper |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | K-Max |
| Status: | needs review |
Description
Long time Drupal User, first day posting bug reports and patches. :)
Tried testing a conversion with an mpeg-1 file (ie: .mpg file) and it failed. Found that it was using the file extension to determine file format which isn't a good way to detect formats.
So, I did some work on ffmpeg_wrapper_can_decode and ffmpeg_wrapper_file_data. It uses some tighter patterns to grab more file formats and codecs to get a more accurate read on a file format.
Should improve the info passed back up from the wrapper functions, especially on the testing conversions. Give 'er a try. Should be a fairly transparent patch.
| Attachment | Size |
|---|---|
| ffmpeg_wrapper.patch_01.patch | 5.57 KB |
#1
Need the patch reviewed... heh.
#2
Your concept is really good- should have been doing this from the beginning. I change the approach slightly- let me know if it works for you.
#3
Made the title clearer and more concise...
Thanks! Yeah, I'm just getting my head around getting involved in open-source. So I'm fairly new to submitting patches and stuff.
I'll give it a try. One question through, which revision am I applying this patch to? My original patch was for the module version (from the file) 1.1.2.20.2.14 2009/01/02 20:26:20. When I apply this patch to the module after my patch I do get errors... Sadly I haven't been using CVS yet (lol). I use the patch command.
Hunk #2 FAILED at 336.Hunk #3 succeeded at 542 (offset 35 lines).
Hunk #4 succeeded at 550 (offset 35 lines).
Hunk #5 FAILED at 586.
Hunk #6 succeeded at 602 (offset 35 lines).
Hunk #7 succeeded at 609 (offset 36 lines).
Hunk #8 succeeded at 619 (offset 36 lines).
Hunk #9 succeeded at 627 (offset 36 lines).
Hunk #10 succeeded at 638 (offset 36 lines).
Hunk #11 succeeded at 647 (offset 36 lines).
Hunk #12 succeeded at 829 (offset 35 lines).
Hunk #13 succeeded at 956 (offset 35 lines).
Also, at first glance this line in the patch looks like it has a couple spelling errors.
$file_data = fffmpeg_wrapper_filde_data($path);#4
Yep, spelling mistake on my end. Sorry! FYI, there is a good CVS tutorial here: http://drupal.org/handbook/cvs/quickstart
#5
Ah ok. Does your patch work with the current 6.x-1.x dev or was it already committed? I'll just copy/paste the revised code on the latest dev version and make a new patch from there. Sadly, my free time on this is very limited at the moment.
#6
I didn't commit it yet- I was trying to figure out if that was what you were aiming for. I had to do a bunch of refactoring of how ffmpeg_wrapper generates it's list of file formats because some formats and files list multiple formats- each of these had to be broken out into specific items.
#7
Ok. I'll work off the 6.x-1.x-dev on the weekend and go from there including the code in the patch in #2 as well. I'll keep you posted.
#8
Switching status.
#9
I wasn't able to get to it this weekend. Maybe next weekend hopefully. (I eventually have to get this done as I'm releasing a site soon, lol).
#10
Here it is. The codec detection should be much more solid than ever now. I noticed that ffmpeg also had the color space info included as well and had accounted for different situations when it included the color space or not and also if it included the bit rate of the video or not. (I haven't seen cases when the color space was included and the bitrate was not)
Apply this patch to 6.x-1.x-dev that was posted on March 17th on the project page.
#11
Getting closer!
So I think we're moving toward a solution. One piece that I don't like about your code is the following:
if (stristr($file_type, $file_data['format']) || $file_data['format'] == 'mov,mp4,m4a,3gp,3g2,mj2') {
The reason why I don't this is that ffmpeg may not able able to decode these formats by default- they have to be compiled in (at least last time I did it), so given that we're dealing with multiple versions of ffmpeg, I'd rather rely on getting the list of supported codecs from ffmpeg than hard coding formats which we can't depend on. Since we can keep a cache of these, I think it's worth doing it this way.
So what I'd like to do is either go back to my patch from above and roll your color space checking into that (and/or refactor my hacky code for checking codecs).
How does that sound?
a.
#12
Now I see what you were trying to do in #2. I wasn't sure what was going on.
(Edited original post)
Now that I read #379650: Detection of file formats / codecs should not be by file extension. and re-read #2, I understand a bit better now. :)
Can you post that movie file you tested out which gave you a partial list of file formats like you mentioned in in #379650: Detection of file formats / codecs should not be by file extension. for me to test out on my machine here?
I need to see if it's indeed the same format or not and if it's not the same format, which format it is in.
#13
Any luck yet?
#14
I just committed a modified version of your patch- it's in the dev branch now, so can you grab that and let me know what you think?
#15
It was missing quite a bit of stuff from my earlier patch, like checking the audio and video codecs.
But I took the dev branch, re-added the codec checks and finished it up. I also tightened up the multiple file format checking as well. So with this, it should complete the bug fix. Yahoo!
Run this patch on the dev branch and make sure to clear drupal's cache (or comment out where it reads the data from the cache while testing). It was driving me nuts at some point while running tests earlier because I didn't know it was caching and it was giving me positives on files that should have failed. lol.
#16
All good? *bump*
#17
I took a stab at this- I did a bit of logic cleanup in the ffmpeg_wrapper_can_decode() function to reduce the number of steps it takes to to parse things- or at least it simplifies some things a bit. Let me know what you think
#18
Patch looks good, although I haven't tested it yet. There are a couple of whitespace changes that aren't really relevant to the issue, and there is some confusion around whether or not you should put spaces after exclamation marks when negating expressions (!$var vs. ! $var). But those are minory issues.
#19
Looks all good to me arthurf. Ran my test bed on flv conversions and they all showed the expected results.
One comment about the TODO comment on this block of code:
// if we have na values for the audio and video codecs, the file can not be decoded// @ TODO is this really true? The logic below does not seem to indicate this,
// however, I might not understand what the 'na' value really means
if ($file_data['audio']['codec'] == 'na' && $file_data['video']['codec'] == 'na') {
return false;
}
'na' means that the module (VIA ffmpeg) doesn't recognize a codec correct? So, if the module isn't able to recgonize BOTH the audio and video codecs, then there isn't anything ffmpeg can convert. So you want to return FALSE.
#20
So, we're all good with the testing?
#21
I just committed some of this patch, I'm going to keep reviewing it and try to get the rest of this in shortly
#22
Regarding #11, I believe the line
$file_data['format'] == 'mov,mp4,m4a,3gp,3g2,mj2'is a special case, where ffmpeg return Input 'mov,mp4,m4a,3gp,3g2,mj2' for all extensions listed.
e.g.
Input #0, mov,mp4,m4a,3gp,3g2,mj2, from 'ffmpeg_wrapper/test/test.mov':Duration: 00:00:55.30, start: 0.000000, bitrate: 1244 kb/s
Stream #0.0(eng): Video: qtrle, rgb24, 1014x1074, 30 tbr, 1k tbn, 1k tbc
And there is no Audio listed by default.
BTW, any plan to backport this to D5?