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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Movie import #1258

Merged
merged 20 commits into from May 7, 2020
Merged

Movie import #1258

merged 20 commits into from May 7, 2020

Conversation

scribblemaniac
Copy link
Member

@scribblemaniac scribblemaniac commented Aug 31, 2019

This PR adds the ability to import movies. From the usual import menu, there are now two options, "Movie Video..." and "Movie Audio..." which are meant to be used to extract the frames to a bitmap layer and the audio to a sound layer respectively.

Honestly I was feeling a bit lazy while doing this so I know there is room for improvement. My priority was just getting a working version ready for the next release as this is a very helpful feature for those users who aren't already experts with ffmpeg 馃槈 There is still the opportunity to add an import dialog with additional features to this such as: only importing from a specific start and end time (easy for ffmpeg), audio/video channel selection, or even some kind of preview. There could be some progress bar for the audio import, but you'd have to parse the duration out from the ffmpeg output which is not as easy as the frame number for the video import. The error messages are very unhelpful. There are a few things in the import code I came across that probably need refactoring, particularly moving some things out of mainwindow. Just ideas for the future, I personally think this should be good enough for use as-is.

Closes #1070

@davidlamhauge
Copy link
Contributor

davidlamhauge commented Aug 31, 2019

It is a very basic implementation, but it looks very promising.
When testing it, I accidentally started importing a 10 minute movie, and I had no chance to abort it. This should be fixed.
Otherwise I haven't found problems, but there is room for improvement.
I've not done a thorough test, but aside from the ability to abort the import, there are nothing wrong with merging this, and see what happens when the users use it.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Aug 31, 2019

Excellent work! This has been a most requested over the years, so it is a welcome addition!

I think however that this one could wait at least for a progress bar with a cancel button before merging. Indeed having an import dialog with preview and trimming options (start / end time) would be great for future enhancements, but for the bare minimum of visual feedback, a progress bar, and the ability to cancel the operation are required to communicate to the user what is happening and that there is are no (obvious) errors, or if there's a user mistake to roll back the operation before waiting too long (as David commented).

I honestly can't tell you how many times I've thought a program has locked up because I didn't see visual feedback for long import operations and closed the program thinking it crashed 馃槀

With the new undo / redo scheme this should also have an accompanying action that rolls back all the imported frames, or reapplies the imported frames for a movie clip in case there was an unattended user error and the amount of frames is too long to delete manually frame-by-frame (and because consistency).

Since I know your time is limited, maybe if someone else want to contribute to your branch and consequently to this PR to add these qualifying specs, it should be possible. Either way thanks again for tackling this, hopefully the others can chime in with their comments 馃檹

@scribblemaniac
Copy link
Member Author

scribblemaniac commented Aug 31, 2019

All the operations are practically cancellable now.

Copy link
Member

@MrStevns MrStevns left a comment

It's a nice addition for sure but the fact that we're getting another dependency, though static, will nudge the application size 70 mb or so extra... this will probably take pencil2d into 200 mb range, which I'm not exactly excited about. The application is still relatively simple and yet the size of the program enter the range of older adobe software.

@scribblemaniac
Copy link
Member Author

scribblemaniac commented Sep 1, 2019

@candyface There's plenty of room for improvement in that regard for whomever is adventurous enough. The ffmpeg/ffprobe binaries could be decreased significantly by compiling a more minimal version. The Qt components could probably be reduced with Qt Lite. You could remove the ffprobe dependency entirely if you parsed the duration from the ffmpeg output, but that would be a pain and much more prone to breaking in the future.

Of the things we can optimize with the application (speed, memory, and size), I think size is the least important of them. Of course we shouldn't be reckless with it, but space is a fairly abundant resource for most modern systems, and app sizes are naturally larger than they used to be.

@scribblemaniac
Copy link
Member Author

scribblemaniac commented Sep 7, 2019

I've sweetened the deal 馃榿 Now this also adds support for many more audio file formats (*.wma *.ogg *.flac *.opus *.aiff *.aac *.caf) and "fixes" the mp3 playback issue on windows by converting it to a wav on import.

@MrStevns
Copy link
Member

MrStevns commented Sep 8, 2019

I've sweetened the deal 馃榿 Now this also adds support for many more audio file formats (*.wma *.ogg *.flac *.opus *.aiff *.aac *.caf) and "fixes" the mp3 playback issue on windows by converting it to a wav on import.

Awesome. I think it was always a good idea to convert the audio files to wav internally when importing, to make sure codecs wouldn't be a problem. The user doesn't notice anyway so it just makes it easier for us.

This doesn't make use of FFprobe though does it?

Copy link
Member

@MrStevns MrStevns left a comment

LGTM code wise. Haven't tested it.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Sep 8, 2019

@scribblemaniac Awesome! A quick question though, will these formats be only for videos with audio containers with those codecs or will this improvement expand to the sound import dialog as well? (I hope it does 馃檹 )

@scribblemaniac
Copy link
Member Author

scribblemaniac commented Sep 8, 2019

This doesn't make use of FFprobe though does it?

No it does not, but that's mostly because there is no progress bar for sound import.

A quick question though, will these formats be only for videos with audio containers with those codecs or will this improvement expand to the sound import dialog as well?

They are are part of the Import > Sound action, but they use the same function as the movie audio import behind the scenes. It should work with virtually any audio codec in videos already.

Copy link
Contributor

@davidlamhauge davidlamhauge left a comment

Apart from the comment on a maximum amount of frames, I have no issues here.
LGTM.

return false;
}

// Get frame estimate
Copy link
Contributor

@davidlamhauge davidlamhauge Sep 9, 2019

Choose a reason for hiding this comment

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

You make a frame estimate here. I think it should be used to limit the amount of frames to be imported.
I imported a mp4, that lasted 13 seconds. It took a minute.
After that I tried to import a mp4 that lasted 3:56 minutes. After 4-5 minutes it started importing pictures. After importing about 350 frames, the software crashed.
What is the biggest video you've tried to import?
Shouldn't we set a maximum of 300-500 frames?

Copy link
Member Author

@scribblemaniac scribblemaniac Sep 14, 2019

Choose a reason for hiding this comment

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

There is no technical limit. If you are on Windows, it probably crashed because you ran out of memory, but that's entirely dependent on the system and is difficult for us to set a good limit on. I can't test this right now, but I will later.

One solution is to output the frame from ffmpeg directly to the temporary folder (or moving files into the temporary folder), which prevents having to have all the frames loaded into memory and would cut the import time in half, however this would not be good for pcl files where writing to the temporary folder is practically the equivalent of saving.

Copy link
Member

@MrStevns MrStevns Sep 14, 2019

Choose a reason for hiding this comment

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

We could load in chunks at a time, since we can't rely on people understanding that their video might generates thousands of images, this will still take a hit in loading time but imo. it's better than crashing the program and won't be as bad as moving frames to the temporary folder for each frame.

@scribblemaniac scribblemaniac added this to the 0.6.5 milestone Sep 13, 2019
@scribblemaniac scribblemaniac added the 馃敺 Major PR (two reviewers when possible) label Sep 13, 2019
Also allow only the text of the image import filter to be
translated.
I'm not in favor of relying upon ffmpeg for retrieving the
duration. It is more difficult to parse and highly suseptible to
changes in format between ffmpeg versions and possibly
locatalizations. I have made the ffmpeg duration code a fallback
if ffprobe is absent, that way if it does cause issues, we can
just tell users to throw the ffprobe binary in the plugins folder
and it will just work.
@scribblemaniac
Copy link
Member Author

scribblemaniac commented Sep 14, 2019

FFprobe is optional now 馃槕

@MrStevns
Copy link
Member

MrStevns commented Sep 28, 2019

Can be merged as soon as the conflicts has been resolved.

@scribblemaniac
Copy link
Member Author

scribblemaniac commented Sep 28, 2019

I will resolve the conflicts soon, but I'm actually going to move this to [WIP] for now. I have an idea to reduce the memory burden of this feature.

@scribblemaniac scribblemaniac changed the title Movie import [WIP] Movie import Sep 28, 2019
@Jose-Moreno Jose-Moreno requested a review from chchwy Dec 16, 2019
@Jose-Moreno Jose-Moreno modified the milestones: 0.6.5, 0.6.6 Jan 15, 2020
This change is so that all frames that have not been modified since
importing from a movie can be unloaded from memory and the frame
cache. Saving is done by copying files on the disk.

This should prevent out of control memory consumption during movie
import, and should make saving said frames very fast.
The previous method worked on metadata tags as far as I can tell,
which are often not present in the output. This uses the stream
duration which should be about as reliable as you can get with
the wrong tool.
@scribblemaniac scribblemaniac modified the milestones: 0.6.6, 0.6.5 Mar 4, 2020
@scribblemaniac scribblemaniac changed the title [WIP] Movie import Movie import Mar 4, 2020
@scribblemaniac
Copy link
Member Author

scribblemaniac commented Mar 4, 2020

I've just implemented the idea mentioned earlier. Frames are saved to the disk right away (but are not part of the project file until it is actually saved). This means lower memory usage, less crashing, and faster saving. Unfortunately this only works for frames being added to a space without an existing keyframe and there's no way around that since the import frame has to be merged with the existing frame.

I've also resolved the merge conflicts. I believe this feature is ready to be added. There are a few quality of life improvements I would still like to see added including import settings similar to the image sequence import (spacing, alignment, ...), better temporary folder cleanup, better error handling, and warnings for large imports, however I feel that this is good enough now that it should be added to v0.6.5. The new audio import is particularly important with many of our users still having issues with mp3 files. Given that @davidlamhauge and @candyface have already reviewed most of this, I am hopeful that it will not take too long to finish reviewing.

@MrStevns
Copy link
Member

MrStevns commented Mar 4, 2020

Awesome, I will review it today.

Copy link
Member

@MrStevns MrStevns left a comment

In general I think it okay, personally I think that error cases should be handled before merging as we otherwise have no way of helping our users if movie import/export fails in a non debug build.

if we create a follow-up issue that highlights the TODO's and the missing error handling, then I'm fine with that too.

MrStevns and others added 4 commits May 4, 2020
鈥usiness logic

This was a bit more difficult than anticipated but the result now is that the process is completely independent of the progress dialog and instead can notify UI if required. Bonus is also that we avoid polluting Editor further.
Not just a refactor though, I also fixed some potential bugs
and improved error logging with DebugDetails and ErrorDialog.
if(st == Status::CANCELED)
{
QMessageBox::warning(mParent,
st.title(),
st.description(),
QMessageBox::Ok,
QMessageBox::Ok);
}
else if (!st.ok())
{
ErrorDialog errorDialog(st.title(), st.description(), st.details().html(), mParent);
errorDialog.exec();
}
Copy link
Member

@MrStevns MrStevns May 7, 2020

Choose a reason for hiding this comment

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

This is not correct, now the user will see an empty dialog every time they cancel the operation. There's no explanation, just a window with a exclamation mark. The reason it was "!st.ok() && st != Status::CANCEL" is because it's only meant to trigger a popup if the operation fails.

Right now you'll see this if you cancel, canceling is already an explicit action, we don't need further confirmation that it's done, therefore it should be silent.
image

Copy link
Member Author

@scribblemaniac scribblemaniac May 7, 2020

Choose a reason for hiding this comment

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

Yeah, I was trying to get a message to display when it was cancelled because it exceeds the frame limit so I thought I'd change the user cancelled events to SAFE, but I did not realize how many points there were where it could be cancelled by the user. And I also didn't want to use ErrorDialog for it because we don't need the extra debug information for importing something that is too long. I have updated it with a solution that should work better. Let me know what you think.

if(st == Status::CANCELED)
{
QMessageBox::warning(mParent,
st.title(),
st.description(),
QMessageBox::Ok,
QMessageBox::Ok);
}
else if (!st.ok())
{
ErrorDialog errorDialog(st.title(), st.description(), st.details().html(), mParent);
errorDialog.exec();
}
Copy link
Member

@MrStevns MrStevns May 7, 2020

Choose a reason for hiding this comment

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

Same as above..

ErrorDialog errorDialog(st.title(), st.description(), st.details().html(), mParent);
errorDialog.exec();
}
return st;
Copy link
Member

@MrStevns MrStevns May 7, 2020

Choose a reason for hiding this comment

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

Same as above

@@ -25,6 +25,7 @@ GNU General Public License for more details.
#include <QImageReader>
#include <QDragEnterEvent>
#include <QDropEvent>
#include <QProgressDialog>
Copy link
Member

@MrStevns MrStevns May 7, 2020

Choose a reason for hiding this comment

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

We should remove this again

@@ -27,6 +27,8 @@ GNU General Public License for more details.

class QDragEnterEvent;
class QDropEvent;
class QProgressDialog;
Copy link
Member

@MrStevns MrStevns May 7, 2020

Choose a reason for hiding this comment

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

This too should be removed

Copy link
Member

@MrStevns MrStevns left a comment

Nice improvement to the error handling on top of my own changes, the latest commit introduced a bug however that needs to be fixed before we can merge. See comments :)

Copy link
Member

@MrStevns MrStevns left a comment

LGTM!

Thanks for making this and for the patience, I'm certain the users will find it valuable :)

@MrStevns MrStevns merged commit fef4858 into pencil2d:master May 7, 2020
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃敺 Major PR (two reviewers when possible)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement video import
4 participants