Skip to content
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

Thumbnail images, folder images, and support for special chars in filenames #13

Closed
wants to merge 21 commits into from

Conversation

Rylab
Copy link
Contributor

@Rylab Rylab commented Mar 10, 2014

This contains everything from last pull request re: async thumbnail generation, as well as an (open source) folder image for directories, and safe handling of special chars in filenames in several different places.

@craiig
Copy link
Owner

craiig commented Mar 10, 2014

This looks great! I like how you added the thumbnail generation in an async way so that the listing gets returned right away. What do you think about putting the thumbs directory in a more central place? I don't know that it's a good idea to generate lots of files and directories in users' media directories (that they would later need to clean up). Maybe put all the thumbs in one spot, like [install_location]/generated_thumbs?

@Rylab
Copy link
Contributor Author

Rylab commented Mar 10, 2014

Yeah, I was considering that too, the only thing I was concerned about was what happens when there are videos with the same name (e.g. S01E01.mp4) -- also, if you ever change your media root, all the relative image paths stored in a central location would break. So, we would need to include the full absolute path to the source videos in an index in the central location, which I guess is possible, it would just be more difficult to get totally right.

I kinda like how currently, if you move around folders, the .thumbs stay with them and continue working, with no complicated index to keep track of. Perhaps I can include an uninstall script which will delete all the .thumbs folders recursively within the media root? Or, if you have an idea for a good way to manage a central location that doesn't have any drawbacks when the media or root moves around, I can definitely look into doing it that way instead!

On Mar 10, 2014, at 10:09 AM, craiig notifications@github.com wrote:

This looks great! I like how you added the thumbnail generation in an async way so that the listing gets returned right away. What do you think about putting the thumbs directory in a more central place? I don't know that it's a good idea to generate lots of files and directories in users' media directories (that they would later need to clean up). Maybe put all the thumbs in one spot, like /generated_thumbs?


Reply to this email directly or view it on GitHub.

@mr-c
Copy link
Contributor

mr-c commented Mar 10, 2014

Why not use a hash of the contents of the movie as the filename and store
in a local directory? That will be resilient to renaming and moving.

Bonus would be to use any existing OS or third party thumbnails that
already exist.

On Mon, Mar 10, 2014 at 2:04 PM, Ryan notifications@github.com wrote:

Yeah, I was considering that too, the only thing I was concerned about was
what happens when there are videos with the same name (e.g. S01E01.mp4) --
also, if you ever change your media root, all the relative image paths
stored in a central location would break. So, we would need to include the
full absolute path to the source videos in an index in the central
location, which I guess is possible, it would just be more difficult to get
totally right.

I kinda like how currently, if you move around folders, the .thumbs stay
with them and continue working, with no complicated index to keep track of.
Perhaps I can include an uninstall script which will delete all the .thumbs
folders recursively within the media root? Or, if you have an idea for a
good way to manage a central location that doesn't have any drawbacks when
the media or root moves around, I can definitely look into doing it that
way instead!

On Mar 10, 2014, at 10:09 AM, craiig notifications@github.com wrote:

This looks great! I like how you added the thumbnail generation in an
async way so that the listing gets returned right away. What do you think
about putting the thumbs directory in a more central place? I don't know
that it's a good idea to generate lots of files and directories in users'
media directories (that they would later need to clean up). Maybe put all
the thumbs in one spot, like /generated_thumbs?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37213958
.

@craiig
Copy link
Owner

craiig commented Mar 10, 2014

The trouble is that if we need to hash the file contents to find the
thumbnail, then thumbnail lookups are going to be slow. It might even be
true that re-running ffmpeg is faster than running md5sum over the file..

What if we store the thumbnails indexed by their location relative to the
media directory? (i.e. media/tvshow1/e01.mp4). It's true that they will get
rebuilt when a rename happens, but I think this will be rare and something
the users don't mind happening. It's a tough tradeoff overall, but I think
it's OK to rebuild when the file gets moved/renamed. Even the current
approach has to rebuild if the file is renamed.

Overall the central location could be [install dir]/generated_thumbs/. If
the file was indexed as "media/tvshow1/e01.mp4" in the media directory,
then we would store the thumb at: "[install
dir]/generated_thumbs/media/tvshow1/e01.mp4.jpg"

Thoughts?

Also, we may also want to consider deleting thumbs when we detect the file
isn't present anymore.

On Mon, Mar 10, 2014 at 11:12 AM, Michael R. Crusoe <
notifications@github.com> wrote:

Why not use a hash of the contents of the movie as the filename and store
in a local directory? That will be resilient to renaming and moving.

Bonus would be to use any existing OS or third party thumbnails that
already exist.

On Mon, Mar 10, 2014 at 2:04 PM, Ryan notifications@github.com wrote:

Yeah, I was considering that too, the only thing I was concerned about
was

what happens when there are videos with the same name (e.g. S01E01.mp4)

also, if you ever change your media root, all the relative image paths
stored in a central location would break. So, we would need to include
the
full absolute path to the source videos in an index in the central
location, which I guess is possible, it would just be more difficult to
get
totally right.

I kinda like how currently, if you move around folders, the .thumbs stay
with them and continue working, with no complicated index to keep track
of.
Perhaps I can include an uninstall script which will delete all the
.thumbs
folders recursively within the media root? Or, if you have an idea for a
good way to manage a central location that doesn't have any drawbacks
when
the media or root moves around, I can definitely look into doing it that
way instead!

On Mar 10, 2014, at 10:09 AM, craiig notifications@github.com wrote:

This looks great! I like how you added the thumbnail generation in an
async way so that the listing gets returned right away. What do you think
about putting the thumbs directory in a more central place? I don't know
that it's a good idea to generate lots of files and directories in users'
media directories (that they would later need to clean up). Maybe put all
the thumbs in one spot, like /generated_thumbs?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/craiig/HOTDOGSEAGULL/pull/13#issuecomment-37213958>

.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37214922
.

@mr-c
Copy link
Contributor

mr-c commented Mar 10, 2014

FYI: if the file can fit in memory than a subsequent md5sum will be fast
due to the built in caching behavior of Linux

On Mon, Mar 10, 2014 at 2:23 PM, craiig notifications@github.com wrote:

The trouble is that if we need to hash the file contents to find the
thumbnail, then thumbnail lookups are going to be slow. It might even be
true that re-running ffmpeg is faster than running md5sum over the file..

What if we store the thumbnails indexed by their location relative to the
media directory? (i.e. media/tvshow1/e01.mp4). It's true that they will get
rebuilt when a rename happens, but I think this will be rare and something
the users don't mind happening. It's a tough tradeoff overall, but I think
it's OK to rebuild when the file gets moved/renamed. Even the current
approach has to rebuild if the file is renamed.

Overall the central location could be [install dir]/generated_thumbs/. If
the file was indexed as "media/tvshow1/e01.mp4" in the media directory,
then we would store the thumb at: "[install
dir]/generated_thumbs/media/tvshow1/e01.mp4.jpg"

Thoughts?

Also, we may also want to consider deleting thumbs when we detect the file
isn't present anymore.

On Mon, Mar 10, 2014 at 11:12 AM, Michael R. Crusoe <
notifications@github.com> wrote:

Why not use a hash of the contents of the movie as the filename and store
in a local directory? That will be resilient to renaming and moving.

Bonus would be to use any existing OS or third party thumbnails that
already exist.

On Mon, Mar 10, 2014 at 2:04 PM, Ryan notifications@github.com wrote:

Yeah, I was considering that too, the only thing I was concerned about
was

what happens when there are videos with the same name (e.g. S01E01.mp4)

also, if you ever change your media root, all the relative image paths
stored in a central location would break. So, we would need to include
the
full absolute path to the source videos in an index in the central
location, which I guess is possible, it would just be more difficult to
get
totally right.

I kinda like how currently, if you move around folders, the .thumbs
stay
with them and continue working, with no complicated index to keep track
of.
Perhaps I can include an uninstall script which will delete all the
.thumbs
folders recursively within the media root? Or, if you have an idea for
a
good way to manage a central location that doesn't have any drawbacks
when
the media or root moves around, I can definitely look into doing it
that
way instead!

On Mar 10, 2014, at 10:09 AM, craiig notifications@github.com wrote:

This looks great! I like how you added the thumbnail generation in an
async way so that the listing gets returned right away. What do you
think
about putting the thumbs directory in a more central place? I don't
know
that it's a good idea to generate lots of files and directories in
users'
media directories (that they would later need to clean up). Maybe put
all
the thumbs in one spot, like /generated_thumbs?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/craiig/HOTDOGSEAGULL/pull/13#issuecomment-37213958>

.

Reply to this email directly or view it on GitHub<
https://github.com/craiig/HOTDOGSEAGULL/pull/13#issuecomment-37214922>

.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37216209
.

@Rylab
Copy link
Contributor Author

Rylab commented Mar 10, 2014

I guess that's true, changing the media root would probably be pretty rare in normal use. I like the approach of just mimicking the relative path within the media root, within generated_thumbs -- no md5 hashing or anything, it won't be necessary using this pattern. I'll use the same "videofile.thumb.jpg" naming I use now, but within a top-level hdsg "generated_thumbs" directory rather than directly inside the users' media folders. I can also look into adding a cleanup script that loops through and deletes thumbnails for videos that are no longer present.

On Mar 10, 2014, at 11:23 AM, craiig notifications@github.com wrote:

The trouble is that if we need to hash the file contents to find the
thumbnail, then thumbnail lookups are going to be slow. It might even be
true that re-running ffmpeg is faster than running md5sum over the file..

What if we store the thumbnails indexed by their location relative to the
media directory? (i.e. media/tvshow1/e01.mp4). It's true that they will get
rebuilt when a rename happens, but I think this will be rare and something
the users don't mind happening. It's a tough tradeoff overall, but I think
it's OK to rebuild when the file gets moved/renamed. Even the current
approach has to rebuild if the file is renamed.

Overall the central location could be [install dir]/generated_thumbs/. If
the file was indexed as "media/tvshow1/e01.mp4" in the media directory,
then we would store the thumb at: "[install
dir]/generated_thumbs/media/tvshow1/e01.mp4.jpg"

Thoughts?

Also, we may also want to consider deleting thumbs when we detect the file
isn't present anymore.

On Mon, Mar 10, 2014 at 11:12 AM, Michael R. Crusoe <
notifications@github.com> wrote:

Why not use a hash of the contents of the movie as the filename and store
in a local directory? That will be resilient to renaming and moving.

Bonus would be to use any existing OS or third party thumbnails that
already exist.

On Mon, Mar 10, 2014 at 2:04 PM, Ryan notifications@github.com wrote:

Yeah, I was considering that too, the only thing I was concerned about
was

what happens when there are videos with the same name (e.g. S01E01.mp4)

also, if you ever change your media root, all the relative image paths
stored in a central location would break. So, we would need to include
the
full absolute path to the source videos in an index in the central
location, which I guess is possible, it would just be more difficult to
get
totally right.

I kinda like how currently, if you move around folders, the .thumbs stay
with them and continue working, with no complicated index to keep track
of.
Perhaps I can include an uninstall script which will delete all the
.thumbs
folders recursively within the media root? Or, if you have an idea for a
good way to manage a central location that doesn't have any drawbacks
when
the media or root moves around, I can definitely look into doing it that
way instead!

On Mar 10, 2014, at 10:09 AM, craiig notifications@github.com wrote:

This looks great! I like how you added the thumbnail generation in an
async way so that the listing gets returned right away. What do you think
about putting the thumbs directory in a more central place? I don't know
that it's a good idea to generate lots of files and directories in users'
media directories (that they would later need to clean up). Maybe put all
the thumbs in one spot, like /generated_thumbs?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/craiig/HOTDOGSEAGULL/pull/13#issuecomment-37213958>

.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37214922
.


Reply to this email directly or view it on GitHub.

@craiig
Copy link
Owner

craiig commented Mar 10, 2014

Ryan - Sounds great.

Michael - I don't know that it's likely that the user's entire media
directory will present in the file cache, but the same caching behaviour
would also speed up ffmpeg as well. :)

C

On Mon, Mar 10, 2014 at 11:31 AM, Ryan notifications@github.com wrote:

I guess that's true, changing the media root would probably be pretty rare
in normal use. I like the approach of just mimicking the relative path
within the media root, within generated_thumbs -- no md5 hashing or
anything, it won't be necessary using this pattern. I'll use the same
"videofile.thumb.jpg" naming I use now, but within a top-level hdsg
"generated_thumbs" directory rather than directly inside the users' media
folders. I can also look into adding a cleanup script that loops through
and deletes thumbnails for videos that are no longer present.

On Mar 10, 2014, at 11:23 AM, craiig notifications@github.com wrote:

The trouble is that if we need to hash the file contents to find the
thumbnail, then thumbnail lookups are going to be slow. It might even be
true that re-running ffmpeg is faster than running md5sum over the
file..

What if we store the thumbnails indexed by their location relative to
the
media directory? (i.e. media/tvshow1/e01.mp4). It's true that they will
get
rebuilt when a rename happens, but I think this will be rare and
something
the users don't mind happening. It's a tough tradeoff overall, but I
think
it's OK to rebuild when the file gets moved/renamed. Even the current
approach has to rebuild if the file is renamed.

Overall the central location could be [install dir]/generated_thumbs/.
If
the file was indexed as "media/tvshow1/e01.mp4" in the media directory,
then we would store the thumb at: "[install
dir]/generated_thumbs/media/tvshow1/e01.mp4.jpg"

Thoughts?

Also, we may also want to consider deleting thumbs when we detect the
file
isn't present anymore.

On Mon, Mar 10, 2014 at 11:12 AM, Michael R. Crusoe <
notifications@github.com> wrote:

Why not use a hash of the contents of the movie as the filename and
store
in a local directory? That will be resilient to renaming and moving.

Bonus would be to use any existing OS or third party thumbnails that
already exist.

On Mon, Mar 10, 2014 at 2:04 PM, Ryan notifications@github.com
wrote:

Yeah, I was considering that too, the only thing I was concerned
about
was
what happens when there are videos with the same name (e.g.

S01E01.mp4)

also, if you ever change your media root, all the relative image
paths
stored in a central location would break. So, we would need to
include
the
full absolute path to the source videos in an index in the central
location, which I guess is possible, it would just be more difficult
to
get
totally right.

I kinda like how currently, if you move around folders, the .thumbs
stay
with them and continue working, with no complicated index to keep
track
of.
Perhaps I can include an uninstall script which will delete all the
.thumbs
folders recursively within the media root? Or, if you have an idea
for a
good way to manage a central location that doesn't have any
drawbacks
when
the media or root moves around, I can definitely look into doing it
that
way instead!

On Mar 10, 2014, at 10:09 AM, craiig notifications@github.com
wrote:

This looks great! I like how you added the thumbnail generation in
an
async way so that the listing gets returned right away. What do you
think
about putting the thumbs directory in a more central place? I don't
know
that it's a good idea to generate lots of files and directories in
users'
media directories (that they would later need to clean up). Maybe
put all
the thumbs in one spot, like /generated_thumbs?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/craiig/HOTDOGSEAGULL/pull/13#issuecomment-37213958>

.

Reply to this email directly or view it on GitHub<
https://github.com/craiig/HOTDOGSEAGULL/pull/13#issuecomment-37214922>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37217188
.

…th; move all options into new config.json; default to top-level thumbnail cache in config.json, but still default to local-level if no config.json present
@Rylab
Copy link
Contributor Author

Rylab commented Mar 13, 2014

Ok, checkout the latest on my fork -- I moved all the configuration options into a new config.json, and replaced the single thumbnail path var with thumb_prefix and thumb_suffix (prefix is within main media_folder option, and suffix is optional). Meaning, my original inner-folder .thumbs, a top-level /.thumbs (new default), and a combo of both qualifies are all easily possible now via the config.json.

@Rylab
Copy link
Contributor Author

Rylab commented Mar 13, 2014

This also knocks another feature request off the list -- we can toss pretty much any config stuff in this config.json which gets required and read into a hash for use everywhere.

…ing thru ffmpeg; refactor ignoredFiles array as server-wide, add imageTypes array in case i missed some
@Rylab
Copy link
Contributor Author

Rylab commented Mar 13, 2014

Added native display of common Chrome-compatible image types (jpg, png, webp, etc) -- set a static width but left height auto to keep original aspect ratio.

@Rylab
Copy link
Contributor Author

Rylab commented Mar 13, 2014

Ok, I'm probably going to come off as a bit OCD, but fixed a crash related to jumping straight into a sub-subdirectory before any thumbnails were created, by requiring the great mkdirp (recursive mkdir) and using the sync version of that to create config-based .thumbs dir before trying to generate thumbs within it.

…dirs before .thumbs folder was created; also made bracket spacing consistent (space before if newline)
@Rylab
Copy link
Contributor Author

Rylab commented Mar 13, 2014

Note, you may (likely) need to do an npm install again to get this new req, but it's worth it, I promise!

@Rylab
Copy link
Contributor Author

Rylab commented Mar 13, 2014

Pretty sure this is all ready to merge back into master, been testing the crap out of it and it's solid.

…ading a new folder, rather than ignoring thumbs until first page refresh; only waits minimum amount needed per # of new thumbnails generated when less than max of 4 seconds
…t, only log actual error messages; add avg initial startup lag of 500ms to first thumbnail creation time, fixes missing thumb images on dirs with < 5 thumbs
…es with certain known-not-image-and-could-do-weird-things extensions; prevents continuous 500ms hang on directories with these common text/script files inside
…ndexes; hide debug div by default on playfile page, link to show again
@craiig
Copy link
Owner

craiig commented Mar 13, 2014

Dude, this looks awesome. I'll check it out after work today and I'll merge
it in! :)

C

On Thu, Mar 13, 2014 at 5:37 AM, Ryan notifications@github.com wrote:

Pretty sure this is all ready to merge back into master, been testing the
crap out of it and it's solid.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37527651
.

@craiig
Copy link
Owner

craiig commented Mar 13, 2014

I just tried this out, and I don't think I've got the right version of fluent-ffmpeg that supports .on(). Do you know which version is right?

Couple things:
Are thumbnails only generated for directories but not for any files in the root media dir?
I think it might be a good idea to make a separate module for thumbnail generation and provide functions like, generate_thumbs(root_dir) and get_thumb(file). It would also be useful to detach thumbnail generation from serving the view and put it into some kind of async thing? That way the views would always be returned fast, and thumbnails will starting showing up whenever they're done.
Thoughts?

@Rylab
Copy link
Contributor Author

Rylab commented Mar 16, 2014

I have fluent-ffmpeg 1.6.0, it was installed by default when I did a normal npm install.

You're right, thumbnails weren't being generated by the root URL, unless you go "back up" to it from a subdir. Do you think it would make more sense to use the same viewfolder function to handle the '/' URL, rather than being a bit different? That's the root cause -- initial load doesn't call viewfolder, which was the only place I added the generation code.

I totally agree serving the thumbnails async inline from the page source, is the ideal way to go. Do you mind if I throw JQuery in the HTML includes, to help with the ajax calls to make that happen? I would just load it from the recommended Google cache (e.g. <script src="//ajax.googleapis.com/ajax/libs/jquery/2.1.0/jquery.min.js">)

On Mar 13, 2014, at 10:37 AM, craiig notifications@github.com wrote:

I just tried this out, and I don't think I've got the right version of fluent-ffmpeg that supports .on(). Do you know which version is right?

Couple things:
Are thumbnails only generated for directories but not for any files in the root media dir?
I think it might be a good idea to make a separate module for thumbnail generation and provide functions like, generate_thumbs(root_dir) and get_thumb(file). It would also be useful to detach thumbnail generation from serving the view and put it into some kind of async thing? That way the views would always be returned fast, and thumbnails will starting showing up whenever they're done.
Thoughts?


Reply to this email directly or view it on GitHub.

@craiig
Copy link
Owner

craiig commented Mar 17, 2014

Ok, I'll check my version of fluent-ffmpeg. It does seem reasonable to use
the same view, for sure.

I'm really unsure about having the browser drive the generation of
thumbnails, but I'm not sure of the other good ways to do background tasks
on node. Maybe it would be best to have the thumbnail generation be a
separate process?

C

On Sat, Mar 15, 2014 at 9:33 PM, Ryan notifications@github.com wrote:

I have fluent-ffmpeg 1.6.0, it was installed by default when I did a
normal npm install.

You're right, thumbnails weren't being generated by the root URL, unless
you go "back up" to it from a subdir. Do you think it would make more sense
to use the same viewfolder function to handle the '/' URL, rather than
being a bit different? That's the root cause -- initial load doesn't call
viewfolder, which was the only place I added the generation code.

I totally agree serving the thumbnails async inline from the page source,
is the ideal way to go. Do you mind if I throw JQuery in the HTML includes,
to help with the ajax calls to make that happen? I would just load it from
the recommended Google cache (e.g. <script src="// ajax.googleapis.com/ajax/libs/jquery/2.1.0/jquery.min.js">)

On Mar 13, 2014, at 10:37 AM, craiig notifications@github.com wrote:

I just tried this out, and I don't think I've got the right version of
fluent-ffmpeg that supports .on(). Do you know which version is right?

Couple things:
Are thumbnails only generated for directories but not for any files in
the root media dir?
I think it might be a good idea to make a separate module for thumbnail
generation and provide functions like, generate_thumbs(root_dir) and
get_thumb(file). It would also be useful to detach thumbnail generation
from serving the view and put it into some kind of async thing? That way
the views would always be returned fast, and thumbnails will starting
showing up whenever they're done.
Thoughts?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37748638
.

@Rylab
Copy link
Contributor Author

Rylab commented Mar 17, 2014

The browser just will check for the generated thumbnails and display them without needing to refresh this way. It's still a normal node server process fired off by the controller before rending the view (via a separate generate_thumbnails function) which does the real work. I'll check in what I have to take a look later tonight so you can see what I mean.

On Mar 17, 2014, at 8:36 AM, craiig notifications@github.com wrote:

Ok, I'll check my version of fluent-ffmpeg. It does seem reasonable to use
the same view, for sure.

I'm really unsure about having the browser drive the generation of
thumbnails, but I'm not sure of the other good ways to do background tasks
on node. Maybe it would be best to have the thumbnail generation be a
separate process?

C

On Sat, Mar 15, 2014 at 9:33 PM, Ryan notifications@github.com wrote:

I have fluent-ffmpeg 1.6.0, it was installed by default when I did a
normal npm install.

You're right, thumbnails weren't being generated by the root URL, unless
you go "back up" to it from a subdir. Do you think it would make more sense
to use the same viewfolder function to handle the '/' URL, rather than
being a bit different? That's the root cause -- initial load doesn't call
viewfolder, which was the only place I added the generation code.

I totally agree serving the thumbnails async inline from the page source,
is the ideal way to go. Do you mind if I throw JQuery in the HTML includes,
to help with the ajax calls to make that happen? I would just load it from
the recommended Google cache (e.g. <script src="// ajax.googleapis.com/ajax/libs/jquery/2.1.0/jquery.min.js">)

On Mar 13, 2014, at 10:37 AM, craiig notifications@github.com wrote:

I just tried this out, and I don't think I've got the right version of
fluent-ffmpeg that supports .on(). Do you know which version is right?

Couple things:
Are thumbnails only generated for directories but not for any files in
the root media dir?
I think it might be a good idea to make a separate module for thumbnail
generation and provide functions like, generate_thumbs(root_dir) and
get_thumb(file). It would also be useful to detach thumbnail generation
from serving the view and put it into some kind of async thing? That way
the views would always be returned fast, and thumbnails will starting
showing up whenever they're done.
Thoughts?

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37748638
.


Reply to this email directly or view it on GitHub.

…ding via ajax on first index; added loading spinner gif; moved all JS to bottom of page so it renders immediately; some more minor UI polish; got rid of separate viewfolder route, refactored into similar / route
…ding via ajax on first index; added loading spinner gif; moved all JS to bottom of page so it renders immediately; some more minor UI polish; got rid of separate viewfolder route, refactored into similar / route
@Rylab
Copy link
Contributor Author

Rylab commented Mar 19, 2014

Ok, checkout my latest commits here. I have seen very occasionally, a thumbnail_src gets returned to the page, but the browser fails to render the image. This seems like some sort of race condition where the fluent-ffmpeg creation process returns true before the file is actually available via the /thumb statics controller. Hitting refresh immediately, the file shows up; hesitant to actually commit an arbitrary timeout, but that does fix it completely as a hack. Looking into that known issue a bit deeper next. Other than that, this is pretty slick!

@Rylab
Copy link
Contributor Author

Rylab commented Mar 19, 2014

Aha, figured it out finally! (fs.existsSync(thumb_dir + thumb_name)) returns true as soon as the file starts getting written by the fluent thread, even if the whole image isn't there yet. Just need to implement a more resilient check for the full image to be there rather than the file to exist.

…ter where i had moved mine thus creating the conflict -- hope that is ok w you
@Rylab
Copy link
Contributor Author

Rylab commented Mar 19, 2014

Just merged your recent commits into this pull since there was a minor merge conflict to work out in my footer-moved js :)

…k success from async job rather than polling; prevents multiple generation requests for same video; adds back large thumb to playfile detail screen; gitignore Eclipse project files; add some missing semis
@Rylab
Copy link
Contributor Author

Rylab commented Mar 19, 2014

Alright, totally rewrote it to generate the thumb via the /thumbgen endpoint rather than in the main directoryList loop. Not only does this fix the initial bug with it sometimes showing a broken image, it takes advantage of the fluent-ffmpeg callback to return the new thumb_src immediately as soon as it's generated, rather than polling stupidly like the first version did. Also now preventing multiple generation calls for the same thumb path (e.g. you refresh the page while it's still generating some, before could thrash, now just smartly waits for the first request to finish generating it). Also, added back large thumb to playfile screen.

…nings from chromecast.js when it wasnt found there; cleaned-up spacing in both html template to be consistent
@craiig
Copy link
Owner

craiig commented Mar 19, 2014

As a user, it works great, the async load of images seems to be the right way to go! Thanks for all the work you are doing. There are some technical issues though:

  1. Overall I think the thumbnail generation needs to be moved into it's own module. Right now, there's tons of thumbnail specific code across the entire project. It doesn't seem like it will be very maintainable by me or usable by someone who wants to use HDSG in their own project.
  • the module should have two functions: a function to handle the thumbnail route (/thumb) and a function that returns the proper thumbnail url when given a path to a media file (or returns null if the file is not applicable). Returning the thumb dimensions is probably not needed and something the UI view can deal with itself. The module can be callled from the chromecast module when it's returning a file list, or by the UI view itself.
  1. The required fluent-ffmpeg version should be >=1.6.0 in package.json to use the on() feature.
  2. Why not keep list of supported types for thumbnails instead of files to be ignored? The ignored list needs to be expanded to cover any possible file extension, but the supported types are just: avi,mp4,mkv,mov. This makes it a lot simpler
  3. The thumbnail directory layout is very strange. Why add all these extra .thumbs suffix and prefix to every directory? Please make the directory relative to the path of the server code and not the media library, that way if someone decides to get rid of HDSG, they can just delete the server directory and get everything in one shot. i.e. thumbnails should be stored at [server_root]/generated_thumbs/[moviename].jpg. If the file is stored in a subdirectory, it should just be: [server_root]/generated_thumbs/[subdir]/[moviename].jpg

Improvements:
4. Users with low-spec machines might want to turn thumbnail generation off entirely, in this case the thumbnail module will always return null.

Once again, thanks for all the work you're doing on this. :)

@Rylab
Copy link
Contributor Author

Rylab commented Mar 20, 2014

All good feedback. 1b-4 are easy improvements, as far as moving it to a module, I'm just getting into Node.JS with this project and unsure if I'll be able to organize it exactly how you're saying without a lot of head banging. Which I guess is a good way to learn, but, I've never done a module like that yet so it may take some time.

@craiig
Copy link
Owner

craiig commented Mar 20, 2014

Sounds good. :) If you like, do 1-4 and I can help to create the module?

C

On Wed, Mar 19, 2014 at 9:21 PM, Ryan notifications@github.com wrote:

All good feedback. 1-4 are easy improvements, as far as moving it to a
module, I'm just getting into Node.JS with this project and unsure if I'll
be able to organize it exactly how you're saying without a lot of head
banging. Which I guess is a good way to learn, but, I've never done a
module like that yet so it may take some time.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-38134133
.

@Rylab
Copy link
Contributor Author

Rylab commented Mar 20, 2014

That would be awesome! Just a heads up, I'm going out of town without internet access for ~10 days starting tomorrow, but I'll pick this up again when I get back :)

@craiig
Copy link
Owner

craiig commented Mar 20, 2014

No problem! :)

C

On Wed, Mar 19, 2014 at 9:25 PM, Ryan notifications@github.com wrote:

That would be awesome! Just a heads up, I'm going out of town without
internet access for ~10 days starting tomorrow, but I'll pick this up again
when I get back :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-38134270
.

@Rylab Rylab closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants