Closed (fixed)
Project:
Media Mover
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
25 Feb 2013 at 01:36 UTC
Updated:
2 Mar 2013 at 00:41 UTC
Jump to comment: Most recent file
Comments
Comment #1
arthurf commentedIs this from media mover or just testing with ffmepg_wrapper? Are other conversions working with ffmpeg_wrapper?
Comment #2
iva2k commentedHere's a patch that fixes ffmpeg thumbnails and adds some error checking and more detailed reporting.
Comment #3
iva2k commentedComment #4
iva2k commentedVideo conversion (kinda) works. There are few glitches with ffmpeg library. If I find something material I will post a patch to either ffmpeg wrapper or mm_ffmpeg.
Comment #5
arthurf commentedVideo conversion should be working with media mover and ffmpeg- provided the version you're using matches up with where ffmpeg_wrapper is- the module needs to be updated to deal with changes to the arguments. Unfortunately ffmpeg_wrapper is poorly architected- I think it needs a serious look to restructure it so it can handle differing versions of ffmpeg more graceully.
Comment #6
iva2k commentedI did not touch video part of mm_ffmpeg module in the above patch. I made video work by providing custom args to ffmpeg to cover the bugs in the ffmpeg. In my sandbox I added more detailed error handling which helped me debug the conversion part, but it did not change the actual functionality. If you want, I can post the video patch here (or in a new issue).
Version handling of ffmpeg would be nice, but I want to get an end-to-end solution first, then I can help with ffmpeg_wrapper. Sounds like using class factory to wrap ffmpeg API can work transparently. I like what was done as an API in phpvideotoolkit (used in video.module). All settings have class methods and command is built from the object parameters. ffmpeg_wrapper already has a class wrapper, so a lot of code can be copy-pasted (if the license allows it).
Comment #7
arthurf commentedMind posting your patch? I tested with mov and 3gp files and didn't have any issues.
Comment #8
iva2k commentedI posted ffmpeg patch on [#1926486#]. It fixes few bugs that may be affecting mm_ffmpeg conversions.
Here's a patch for mm_ffmpeg with the following:
Comment #9
iva2k commentedPatch in #8 has one typo, so don't use it. Here's a fixed one, get_entity_link() is improved. It will be required for the entity support patch I'm preparing for the core in #1926464: mm_fields for Drupal 7.
BTW, why the test bot is stuck in all issues in review?
Comment #10
iva2k commentedComment #11
arthurf commentedOk this looks pretty good. Can you break out the work you did on class_media_mover_files separately? A few random thoughts: seems like what you're doing in load_info() should be done in load_info(). Can you explain what you need get_step_file() for? It seems reasonable but I'm not sure what it would get used for.
Comment #12
iva2k commented> Can you break out the work you did on class_media_mover_files separately?
I can, but 1). I'm really behind my set schedule and the time window when I can work on drupal will close very soon. I want to finish mm_fields.module. 2) Changes in class_media_mover_files are needed for mm_ffmpeg patch. That's why I combined them into one patch. mm_fmpeg depends on additional error handling in media_mover_api_create_local_path().
>Can you explain what you need get_step_file() for?
It is a part of clean encapsulation of the logic that saves step progress in the files table. get_step_file() is already used in get_uri() and get_link() which provide help for displaying steps information in media_mover_ui patch I posted separately and is used in upcoming mm_fields module (there is a lot of scanning of step files and this cleans up a lot of mess that would be otherwise).
Comment #13
arthurf commentedCommitted this with some changes to the new functions in media_mover_files