New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the thumbnail script to MediaHub #305

Merged
merged 11 commits into from Jun 27, 2016

Conversation

Projects
None yet
2 participants
@mfairchild365
Copy link
Contributor

mfairchild365 commented Jun 24, 2016

This removes a dependency on another server/platform and give mediahub more control over how the script works. It also fixes several issues where were found (discussed out of channel).

For deployment I have script to move all of the current thumbnails over. Deployment will look like this:

  1. Copy script to server and run it (this will move over existing thumbnails to the new directory structure)
  2. Run git pull
  3. Verify that everything works as expected
&& isset($_GET['time'])
&& preg_match('/^[\d]+\:[\d]{2}\:[\d]{2}(\.[\d]{2})?$/', $_GET['time'])) {
//Allow customizing the time if the user has permission
$time = $_GET['time'];

This comment has been minimized.

@kabel

kabel Jun 27, 2016

Contributor

This variable should probably also be shell escaped.

@kabel

This comment has been minimized.

Copy link
Contributor

kabel commented Jun 27, 2016

Did you consider routing this new "controller" through the existing index.php?

$url = escapeshellarg($url);
exec(UNL_MediaHub::getRootDir() . "/ffmpeg/ffmpeg -i $url -ss $time -vcodec mjpeg -vframes 1 -f image2 /tmp/posterframe.jpg -y", $return, $status);

This comment has been minimized.

@kabel

kabel Jun 27, 2016

Contributor

Not a huge issue (as we probably don't get much concurrent traffic), but this is not concurrent request safe. A distinct temporary file name should be generated for each request (see tempnam()).

This comment has been minimized.

@mfairchild365

mfairchild365 Jun 27, 2016

Contributor

Good point, I will get this added. We should support it just in case.

}
if ($status == 0) {
$data = file_get_contents('/tmp/posterframe.jpg');

This comment has been minimized.

@kabel

kabel Jun 27, 2016

Contributor

Instead of loading these files into memory, it would be better to use copy() or rename() and readfile().

$directory = UNL_MediaHub::getRootDir() . '/www/uploads/thumbnails/'.$media_id;
$file = $directory.'/original.jpg';
if (isset($_GET['rebuild']) || !file_exists($file)) {

This comment has been minimized.

@kabel

kabel Jun 27, 2016

Contributor

Instead of a giant if wrapper, it would look cleaner to check the opposite and exit (thus avoiding the else case).

@mfairchild365

This comment has been minimized.

Copy link
Contributor

mfairchild365 commented Jun 27, 2016

@kabel I did think about it. My thoughts on the subject:

  • The initial goal was to simply copy/paste the existing script over. However, there was so many things that needed to be fixed in it, that I ended up re-writing it. Keeping it its own script seemed the simplest thing to do.
  • I think it would be more efficient to keep this its own script in order to make things as efficient as possible. If a thumbnail exists for media, a database connection should NOT be established and all the script should do is pass the image through.

I can also see the rational for keeping it in line with the rest of the application, and honestly the performance benefits of keeping it its own script are probably minimal. Do you think it should be routed through the existing index.php?

@kabel

This comment has been minimized.

Copy link
Contributor

kabel commented Jun 27, 2016

I do think it should be with the rest of the app, if only just to keep it DRY (avoid copied code).
If the performance is a concern, there should be optimizations to the index bootstrap.

public function getThumbnail()

This comment has been minimized.

@kabel

kabel Jun 27, 2016

Contributor

It would be better if this method returned a file system path instead of the file data. That way you can send the file without loading it into memory.

This comment has been minimized.

@mfairchild365

mfairchild365 Jun 27, 2016

Contributor

Done-zors

@kabel kabel merged commit f794aad into unl:4.0_template Jun 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment