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

fix for automatic relinking of missing media files during project open #2275

Merged
merged 3 commits into from
Nov 1, 2018

Conversation

fschill
Copy link
Contributor

@fschill fschill commented Oct 24, 2018

(addresses part 1 of issue "Media relinking in file properties #1829" )

This pull request contains some minor changes in project_data.py, to automatically try and relink missing media files after the user has specified a directory. Previous behaviour: a dialog would open multiple times for each missing file, giving the impression that OpenShot is stuck in an infinite loop. New behaviour: After the user picks a directory for the first missing file, the same directory is tried for all other missing files first. Only if that fails, a dialog opens to ask for a new directory.

@peanutbutterandcrackers
Copy link
Contributor

Neato! Will test this out soon! [Glad to see your PR here, BTW]

Thank you for the good work! 👍

@fschill
Copy link
Contributor Author

fschill commented Oct 24, 2018

No worries, and sorry it took me so long. I've had this sitting around for a few months already, I just needed to sort it out and bring it up to date.
I split my changes into two parts - this one is essentially a bugfix for the seemingly endless loop when opening a project with missing files. Only minor changes.

I'll submit the second part shortly, to allow relinking of individual files. I decided to keep it separate as the second part is a feature that can go wrong when the user relinks to a completely different file. More details in the description of part 2...

@DylanC
Copy link
Collaborator

DylanC commented Oct 28, 2018

@peanutbutterandcrackers - Do you have time to test this one? I'm away this weekend from my dev PC.

@peanutbutterandcrackers
Copy link
Contributor

@DylanC - Right on it, Cap'ain!

@fschill - Sorry I couldn't get to this sooner.

Test 1:

File ~/Videos/VideoProject/project.osp was moved to ~/test and was run using this branch. I could preview and render/export the file .osp project. I could even apply new effects on top of the clips on the timeline, and slice them and move them around on the timeline, as well. So, great work!
However,

  1. I wasn't asked to browse for missing files. I didn't even get a complaint about a missing file.
  2. I was greeted with this message twice (for some reason, and my video uses 3 clips made from the same file):
    screenshot at 2018-10-28 17-31-08
  3. There appear to be no thumbnails in the timeline. Perhaps thumnail-regneration should also be done? I don't really know. There was a 'thumbnail' folder created in the current residing place of the .osp file (~/test) - created by OpenShot, it seems. But it was empty.
  4. Is the 'Project Files' tab supposed to be empty?
    screenshot at 2018-10-28 17-36-22
    ** Fig: Problem no. 3 and 4, demonstrated**

Still, this is a LOT better than what we currently have. I strongly advise polishing this PR up, according to test results, and merging it in. Great work, good sir, @fschill ! 👍

Also pinging @ferdnyc and @N3WWN here for further testing.

Next, I want try to test moving a .osp file between users and computers too. And also an external hard-drive.

@fschill
Copy link
Contributor Author

fschill commented Oct 28, 2018

I'm not sure if I fully understand the test done here, I went the other way: I created a new project, added a few media files, saved and closed. I then moved the media files to a new location without moving the project folder. It asks during open for the new location, and as long as the other media files are in the same place, it automatically relinks them (this is in both my pull requests).
In this branch, it is then also possible to copy a media file somewhere else, or re-code it to another codec under a new name, and go into file properties (right-click in the media files view) and link it to the new filename/path. I tested it by recoding a file to a very low bitrate, and when previewing the clip or playing the timeline, it is obvious that the low-bitrate file is playing. It can also be linked back to the original version.

I think in your test with moving the project file, not much happens, as filenames are stored as absolute paths? So when you open the moved project, it still finds the original media files, and this is independent of my changes and should work in the previous version as well. It seems that thumbnails are stored relative to the project file and are not found in this scenario? I can't comment on the internals for this, these are just guesses based on my observations.

Maybe test moving the media files as I described to see if that works for you.

@peanutbutterandcrackers
Copy link
Contributor

Oops. Sorry. My bad.

Test 2:

It works really well. Just point to a directory and it does all the re-linking by itself. Great!

I did try to push it a little too much and did find one thing that would be great if it was fixed.

I created a project inside a separate directory, say, test and put all the files under footage (not the real name, again; the names were lousy). Once I moved the footage directory, with all the footage in there entact, it worked great. However, I moved the directory, and also moved a few footages from inside the footage directory to other directories (~/Video, ~/Desktop). What happened then was, it asked for 82989347hshdjfhasdiyioyh.webm and since I didn't know where exactly that file was, I pointed to one of the 3, which probably happened to be wrong so I ended up having to redo that a couple of times.
I think that scenario is not that uncommon. Only very few of us, who are aware of how unforgiving OpenShot is regarding the placement of our files in the filesystem, take the necessary precaution of keeping a separate folder of footages and keep it without moving. The rest, however, (and this included me too) point to an Image file from here and a video file from there. And even with the re-linking, with the cryptic media file names from our cameras (and social sites cough Facebook cough), it might be worth it to polish this PR to be able to deal with these corner cases as well.
I wonder if we could have a small list of directories saved. Say for file 'A.mp4' user points to dir 'X' but it is not there to be found

Sorry, I just realized that what I was thinking was pretty stupid. However, I am keeping that in here, in case it might give someone any new ideas. For instance, I am starting to think (and I might be being stupid again) that it'd be even easier if we could somehow let a user specify more than one directory at once. But I have no idea how that'll be implemented, esp. with the GUI. sigh

Anyways, I think this PR's great! Thank you @fschill. You're awesome. 👍 💯

P. S: I have a question. I moved the entire project directory - that contained all the footages used, the .osp file, the thumbnails and all - to another place and OpenShot (not this branch) was still able to open it up. But the thumbnails still wouldn't show. It's the same with this branch too. Even after saving the project once it's open. However, the thumbnail directory still shows thumbnails in them. I wonder why that is.
moving projects
Attached is the project, for testing. It has memes. So that should be fun. :)
OpenShot-Media-Relinking-Test.tar.gz

@fschill
Copy link
Contributor Author

fschill commented Oct 28, 2018

Thanks for the testing.

Regarding the issues around moving project files and thumbnails, the discussion thread on the issue already went into that a bit. I think it was suggested that it would be better to store everything as relative paths to the project file (at the very least for any files that are in the same directory or a subdirectory of the project). That way everything could be moved without issues. That's a larger issue though which will have a bigger impact (e.g. handling existing projects after the change, etc.).

Regarding thumbnails, I suppose the easiest would be to trigger a re-creation of everything if there are changes? I don't know how to do that for now, so if anyone wants to put that in that would be great. In the bigger picture, moving a folder with everything in it (project file, thumbnails, assets, media, etc.) should always work, so I think if it doesn't it should be treated as a (mild) bug. As thumbnails and caches are temporary files that openshot creates itself somewhere, it should be referenced with relative paths, and if it's not found should be regenerated automatically.

@DylanC
Copy link
Collaborator

DylanC commented Oct 29, 2018

@peanutbutterandcrackers - That project has such random videos. I love it! 🤣 @fschill - I had a look and it seems to work really nice. Of course I did notice the thumbnail issue but that is probably a separate PR.

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 29, 2018

Wow, it'll be really great to have this feature working. I'm excited to take a look at it, if/when I get a chance.

Just some quick notes:
@fschill :

I think in your test with moving the project file, not much happens, as filenames are stored as absolute paths? So when you open the moved project, it still finds the original media files, and this is independent of my changes and should work in the previous version as well. It seems that thumbnails are stored relative to the project file and are not found in this scenario? I can't comment on the internals for this, these are just guesses based on my observations.

Regarding the issues around moving project files and thumbnails, the discussion thread on the issue already went into that a bit. I think it was suggested that it would be better to store everything as relative paths to the project file (at the very least for any files that are in the same directory or a subdirectory of the project). That way everything could be moved without issues. That's a larger issue though which will have a bigger impact (e.g. handling existing projects after the change, etc.).

Unless something's changed that I'm not aware of, this is exactly backwards.

Currently, when the project file is saved, everything is converted to RELATIVE paths, compared to the project file. When it's read in, they're converted back to absolute, which is why if you're only working with the in-memory strings you'll only ever see absolute paths — but they're made relative before being written to the project file.

(Which is horrible, and why you can't ever move a project file anywhere else, except in the very rare case that all of the media files it uses are stored in a lower directory tree, are moved with it, and all path elements keep the same names. Anything else, the relative paths start with ../ and will almost certainly be invalidated by the move.)

Worse still, the thumbnail paths are also stored relatively, even though they all live under the .openshot_qt/thumbnail/ directory. Which is why, if you move a project file, even recovering the file path probably won't fix the thumbnail path, as ../../../.openshot_qt/thumbnail/RANDOM.png or whatever is no longer the correct path to the thumbnails.

If paths were stored absolutely, then you could move the project file and it wouldn't affect anything at all. And if you moved the media files, you could simply recover their (absolute) paths to the new location. And the thumbnail files would never change their location, so it wouldn't affect those regardless.

(And that's not even touching on the transition paths, which are completely off-topic for this PR. Those are converted to aliased paths like @transitions/common/fade.svg, which works "okay-ish"... BUT, all history entries in saved project files preserve absolute data paths. Because transition files are located inside the OpenShot installation, when using an AppImage build the absolute transition paths will be something like /tmp/.mount_11a1AA/usr/bin/transitions/fade.svg — a string which is randomized at each launch, guaranteeing that the history will become useless as soon as OpenShot is closed, even though it's still preserved in the project file.)

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 29, 2018

@peanutbutterandcrackers :

P. S: I have a question. I moved the entire project directory - that contained all the footages used, the .osp file, the thumbnails and all - to another place and OpenShot (not this branch) was still able to open it up. But the thumbnails still wouldn't show. It's the same with this branch too. Even after saving the project once it's open. However, the thumbnail directory still shows thumbnails in them. I wonder why that is.

Same relative-paths issue. Even when thumbnails are managed correctly by classes/project_data.py, their paths are still stored relative in the project file. So if you move the project file and thumbnail directory together, the thumbnail/*.png files will get picked up and copied to $HOME/.openshot_qt/thumbnail/... but they're still referenced in the project data as ../../../.openshot_qt/thumbnail/RANDOM.png or whatever, so kablooey.

@peanutbutterandcrackers
Copy link
Contributor

@DylanC - Haha. I was on 9gag a little too much.

@ferdnyc - I see. So, a separate PR would be required to fix that? If you could also test this out and confirm it works well, @DylanC could probably proceed to merging it in?

@DylanC
Copy link
Collaborator

DylanC commented Nov 1, 2018

@peanutbutterandcrackers - I don't think we need much more testers on this one. I'm pretty happy with it and how it works currently and you said so too.

@fschill - Thanks for this. I REALLY like this feature and would be happy to have it in by default. One of us can work on the thumbnail re-generation in the meantime. 💯 👍

@DylanC DylanC merged commit 2a4ab70 into OpenShot:develop Nov 1, 2018
@peanutbutterandcrackers
Copy link
Contributor

@DylanC - Great!

@fschill - Thank you for this! You are awesome! :) 👍

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 2, 2018

@peanutbutterandcrackers :

@ferdnyc - I see. So, a separate PR would be required to fix that?

At least one. As I recently noted in an update to #1923: Particularly when AppImages are involved, the history stack presents a severe crash potential...

  • All history-stack file paths are stored absolute
  • ∴ all OpenShot-internal file paths in the history stack are guaranteed to be invalidated
    (AppImage paths expire at the end of the session)
  • None of the paths on the history stack are validated before being applied

All of which means it's very easy to Undo or Redo a saved history action which restores an invalid file path into the current project data, which will cause an immediate traceback and hard crash.

IMHO the history should never be written to the project file at all, and should never be persisted across sessions. Too much of its data potentially has no lifetime beyond the current session. (Not storing history would also decrease the size of project files by 50% or more, in many cases. I have some rough back-of-the-envelope numbers on that in #1923.)

There was a point where I was thinking, "Maybe the backup project data should still store the history, though, for recovery purposes", but that was before I understood how severe the AppImage-internal paths issue was. When running from an AppImage, history isn't ever safe to recover, not even after a crash — the internal file paths will have expired the moment the crash occurred.

path = os.path.join(starting_folder, file_name_with_ext)
# try to find clip with previous starting folder:
if starting_folder and os.path.exists(os.path.join(starting_folder, file_name_with_ext)):
# Update clip path
file["path"] = path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a logic error here — the line that set path to a new value (previous line 920) was deleted, so setting file["path"] = path here is a no-op, since on (old) line 911 path is initially set as path = file["path"].

This should read file["path"] = os.path.join(starting_folder, file_name_with_ext), which is the path that was just tested for existence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submitted #2311 with a quick-fix change for that, but actually maybe I'll try out the new suggest-changes feature:

Suggested change
file["path"] = path
path = os.path.join(starting_folder, file_name_with_ext)
file["path"] = path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Completely unrelated to this code, but as it's currently implemented the new GitHub suggest changes feature is very poor, IMHO. It feels extremely half-assed, and doesn't even give you any context for the line of code you're modifying. #NeedsWork)

@ferdnyc ferdnyc mentioned this pull request Nov 2, 2018
@fschill
Copy link
Contributor Author

fschill commented Nov 2, 2018

Cool, thanks for merging! Happy to contribute.

~~@ferdnyc I'm not sure that the line is redundant. In the first pass of the loop, it is indeed the same value, but as the loop repeats (e.g. if the user specifies different folders after being asked), it would be new values. I don't remember in detail, but I think the idea is that for the next file, it will first try to apply the last specified folder to see if that works, so it won't have to ask the user for each file. ~~

Edit: I think I misunderstood and spoke to fast. Looking at it again, I think I can see now what you mean, it does indeed look weird. I'd have to test it again to make sure it still works, but I assume you've done that. Sorry about that! :)

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 2, 2018

Sorry about that! :)

No worries, this change is great! And already inspired my PR #2312, to fix the thumbnail-recovery issue.

Edit: I think I misunderstood and spoke to fast. Looking at it again, I think I can see now what you mean, it does indeed look weird. I'd have to test it again to make sure it still works, but I assume you've done that.

In my testing, without the path fix (above, or my PR #2311), if I saved a project file to /tmp/test.osp with references to media files in /home/ferd/Videos/, then moved the project file to /var/tmp/test.osp and opened it, OpenShot silently recovered the file locations using import_path (which is awesome!)... it just recovered them as /var/home/ferd/Videos/*, which is obviously not where they really live. (It's also, tellingly, exactly the old stored relative path ../home/ferd/Videos/, when expanded in the new project file directory /var/tmp/ — which is what clued me in to the fact that it wasn't storing the real recovered path.)

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.

4 participants