search

OpenShot / openshot-qt

OpenShot Video Editor is an award-winning free and open-source video editor for Linux, Mac, and Windows, and is dedicated to delivering high quality video editing and animation solutions to the world.
http://www.openshot.org
Other
4.36k stars 544 forks source link

Rejecting valid image files for image sequence import #221

Closed crablab closed 3 years ago

crablab commented 8 years ago

When trying to import multiple .jpg files as an image sequence, OpenShot gives an error saying that they are not valid image, sound or video files (which they are).

Image here: image

opensourcegrappler commented 8 years ago

I also have this error.

Changing the filename of the image seems to fix it (in my case image000.png to foo.png) allows the image to get imported. But if the image is part of an image sequence (it is in my case) the name change prevents the rest of the image sequence from being imported.

DylanC commented 7 years ago

@crablab - Do you get this error in v2.3.4 or in the daily build?

opensourcegrappler commented 7 years ago

I haven't used openshot since I commented on this issue 1 year ago. So I don't know, sorry.

DylanC commented 7 years ago

@crablab - Image sequence is working much better now. You should try v2.4.

zerekw commented 7 years ago

I'm still getting this error in 2.4 on OSX 10.12.6. It fails the first time and then on trying to reimport the file it successfully imports the single image but not the sequence.

zerekw commented 7 years ago

Must be something with the name. Ran an automator script to modify my filenames from "G0011602.JPG" -> "gopro1.JPG". Now everything seems to be working.

zerekw commented 7 years ago

I suspect its something with the number. When I tried to start from a large number, ie. "gopro1234". It choked up again. When I changed the sequence to start from a file named "gopro1.JPG" it worked.

DylanC commented 7 years ago

@zerekw - Yes, sounds like it was designed to work with simple numbers for now.

tahteche commented 7 years ago

In my case an underscore in the file name seems to have the same effect.

dougisfunny commented 6 years ago

From what I can see, it's not so much 'simple' numbers. They just have to start with 0s. This works:

Sequence01.jpg Sequence02.jpg Sequence03.jpg Sequence04.jpg

As does this Sequence04.jpg Sequence05.jpg Sequence06.jpg Sequence07.jpg

This does not

Sequence11.jpg Sequence12.jpg Sequence13.jpg Sequence14.jpg

Nor Does this Sequence05.jpg Sequence06.jpg Sequence07.jpg Sequence08.jpg

Looking through the code I tracked it to files_treeview.py line ~200

                image_seq = openshot.Clip(os.path.join(folder_path, pattern))

                # Update file details
                file.data["path"] = os.path.join(folder_path, pattern)
                file.data["media_type"] = "video"
                file.data["duration"] = image_seq.Reader().info.duration

By adding a traceback extraction I was able to get this,

Sequence10.jpg is not a valid video, audio, or image file. <class 'RuntimeError'> No Reader has been initialized for this Clip. Call Reader(*reader) before calling this method. <traceback object at 0x000000000fb20288> [<FrameSummary file C:\Program Files\OpenShot Video Editor\windows\views\files_treeview.py, line 203 in add_file>, <FrameSummary file C:\msys64\mingw64\lib\python3.6\site-packages\openshot.py, line 1340 in Reader>]

Which I understand is a wrapper for the libopenshot, taking me to Clip.cpp, on line 172 opening a QtImageReader(path) or if that fails the FFmpegReader.cpp at line 178. But that is going a bit far down the rabbit hole, and besides, none of those appear to have anything to do with the image sequence stuff.

Either way, at this point I'm not clear on how, or perhaps more importantly where, the image sequence functionality 'magic' happens to converts the Sequence%02d.jpg to an actual sequence. Or why it only allows numbers lower than 5. So I don't know where it is failing.

DylanC commented 6 years ago

@dougisfunny - Thanks for this extra info. We should look into this further.

caigner commented 5 years ago

I just installed OpenShot on Gentoo Linux and when I start openshot-qt I get a bunch of similar errors.

/usr/lib64/python3.6/site-packages/openshot_qt/effects/icons/bars.png is not a valid image file.
/usr/lib64/python3.6/site-packages/openshot_qt/effects/icons/color shift.png is not a valid image file.
/usr/lib64/python3.6/site-packages/openshot_qt/effects/icons/crop.png is not a valid image file.
...
mikelacsa commented 5 years ago

Tested this with the latest version and it works fine. Closing the issue now.

ferdnyc commented 5 years ago

Tested this with the latest version and it works fine.

Well... it doesn't entirely. Some of the things @dougisfunny brought up are still valid.

This does not

Sequence11.jpg Sequence12.jpg Sequence13.jpg Sequence14.jpg

Nor Does this

Sequence05.jpg Sequence06.jpg Sequence07.jpg Sequence08.jpg

See https://github.com/OpenShot/openshot-qt/issues/2947#issuecomment-524672848 for my explanation of why sequences with starting numbers > 4 will fail to import.

Basically, as I said in that issue, it's an FFmpeg limitation, and I'm not sure what we could really do about it, but it does cause unexpected failures for users who assume that any sequence numbering is valid. (Especially since OpenShot treats any sequence numbering as valid, not checking that the numbering starts low enough.)

jokfish commented 5 years ago

I might not understand the issue properly, but is it possible to use the -start_number flag in FFMPEG? https://ffmpeg.org/faq.html#How-do-I-encode-single-pictures-into-movies_003f It would also be great if the start number wasn't limited to 0-99. Software such as 3dsMax start the numbering at the current frame number in the software, which can easily be in the thousands. Is the get_image_sequence_details function in files_listview.py and files_treeview.py the right place to look?

ferdnyc commented 5 years ago

@jokfish Interesting! That's good to know, thanks .One problem is that we don't use the ffmpeg command line, OpenShot is linked directly with the ffmpeg libraries. But, if there's a way to achieve the same thing using the public API, then libopenshot should be able to take advantage of that, yeah.

I've already concluded that reading the API docs isn't going to shed any light on this subject, since every search I attempt just lands me at files full of completely-undocumented functions, with only the name and the arguments to try and puzzle out what they do. But I'll take a look at the source for the ffmpeg command-line tool, see how they implement --start_number. Hopefully it's not relying on internal API calls or anything.

Is the get_image_sequence_details function in files_listview.py and files_treeview.py the right place to look?

It is (also modified by my #2949, not yet merged), as well as its identical copy-paste cousin in files_treeview.py. (...Don't get me started.)

However, it may not be worth it, because if we are going to support start_number then that entire function is going to have to be thrown out; it's far too inefficient.

Currently it loops over potential filenames, based on the selected one, doing an individual os.path.exists() call for each and every possible match. Fortunately it starts at the selected number - 100, which explains the limitation but is also why files that are numbered "in the thousands" as you say can't completely hang the import process.

If OpenShot is going to have to compute the starting point of an arbitrarily-positioned run of numbers in a set of files, though, looping over os.path.exists() is madness. A whole new method will have to be written that reads in the complete directory listing as a list, then performs list bonsai to produce just the set of filenames required.

But if you're in the mood to write that new method, then by all means! (Just... maybe buck the trend and write it once in src/windows/models/files_model.py, where it BELONGS.)

ferdnyc commented 5 years ago

(If I had my way we wouldn't even bother with all of this terrible code just trying to tease arbitrary sets of filenames out of a directory full of random who-knows-what. Image sequences would be required to live in their own separate directories, one sequence per, and no other files would be permitted to be in there. Then, we could just sort the directory listing numerically, take the first entry, and vioila, there's your start_number.)

jokfish commented 5 years ago

Excellent, I'll take a look when I get a chance. Might just take me a little while to get an idea of the current code. Some thoughts;

ferdnyc commented 5 years ago

Oh, this doesn't look too bad, actually. start_number is one of the muxing parameters used in libavutil/img2enc.c(L210:L219):

#define OFFSET(x) offsetof(VideoMuxData, x)
#define ENC AV_OPT_FLAG_ENCODING_PARAM
static const AVOption muxoptions[] = {
    { "update",       "continuously overwrite one file", OFFSET(update),  AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0,       1, ENC },
    { "start_number", "set first number in the sequence", OFFSET(img_number), AV_OPT_TYPE_INT,  { .i64 = 1 }, 0, INT_MAX, ENC },
    { "strftime",     "use strftime for filename", OFFSET(use_strftime),  AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, ENC },
    { "frame_pts",    "use current frame pts for filename", OFFSET(frame_pts),  AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, ENC },
    { "atomic_writing", "write files atomically (using temporary files and renames)", OFFSET(use_rename), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, ENC },
    { NULL },
};

...Oh, shit. Except, it's not part of the public API. This is what I was afraid of.

$ find /usr/include/ffmpeg -type f -print0 |xargs -0 -n100 grep -i start_num
$

So, that's a complication, unfortunately.

Still, I'll keep looking for solutions. Maybe we can just duplicate the AVOption struct from img2enc.c in our code. The AVOption type itself is part of the public API, and so is AV_OPT_FLAG_ENCODING_PARAM... the rest of it is just string constants, so maybe that's everything we really need.

ferdnyc commented 5 years ago
  • The sequence should stop at the first missing number or gap - It's likely that this is desired behaviour for a couple of reasons.

Not just desired behavior, it's a hard requirement, yeah. FFmpeg will stop as soon as the continuous numbers run out, so there's no choice in the matter.

  • Would it be unreasonable to require that the user selects the first image in the sequence? - That would make processing un-zero-padded sequences a lot easier for one, and actually gives more flexibility (to load a sequence starting in the middle, or skipping the first few frames). This also gives the start_number directly.

Personally I don't think anything's unreasonable (you have no idea how much I hate this feature), but that would be a big change in functionality from what current users are used to, which might be problematic. ...Might not, I don't know, I'd have to find out how users would react to such a change.

But... TBH, setting that restriction really wouldn't buy you anything significant in terms of code simplicity or anything, now that I think about it. Here's why...

Walking the list

Even if we were to require that the user select the first frame in the sequence, OpenShot STILL has to walk the list of files. That's so that it can both:

  1. Determine whether there is indeed an image sequence present. (At least three contiguous numbered filenames are required to qualify as a possibility. And keep in mind, there are plenty of times a user will select a file title_01.jpg to import, from a directory that also contains title_bg.jpg, title2_02.jpg, title002.png, and frame_02.jpg. There's no possible image sequence there, and OpenShot needs to detect that and not offer.)
  2. Also, it needs the opposite boundary of the sequence's image list, so that it knows the length in frames to assign to the imported clip created from that sequence. So, once you've committed to walking the list in one direction regardless, walking it in both directions barely adds any complexity, really.

(And despite the horrible method the current code uses for doing this, if you have a complete list of the filenames, generating the prefix is actually really easy. All you need is the filename from each end of the list.

Pattern generation

Like, say you've determined that your image sequence list is a list of 90 files:

To generate the sequence pattern (in a sane manner), all you need are the start filename myanim2_0021.jpg and the end filename myanim2_0110.jpg.

  1. First you determine whatever prefix string they have in common. (myanim2_0, in this case)
  2. So you chop that prefix off of either filename. (myanim2_0021.jpg => 021.jpg)
  3. Also drop the extension. (021.jpg => 021)
  4. So now that what's left is entirely numeric, you take its length in digits N. (021 => 3)
  5. A printf-style, 0-padded format string, %0Nd, replaces the numeric part. (021 => %03d)
  6. And voila, your pattern is prefix + format_string + extension, or myanim2_0%03d.jpg.

If you implemented the algorithm differently, so that it came up with a pattern of myanim2_%04d.jpg instead? Doesn't matter! If you're passing the start_number to FFmpeg, the exact pattern structure is irrelevant just as long as it's valid.

Pattern flexibility

Even if the user generated those same filenames as myanim20021.jpg through myanim20110.jpg, so you couldn't easily tell which part of the filename is the name of the sequence and which part is the incremental frame-numbering... it actually doesn't matter. Whether you generate the pattern as:

... all three would work exactly the same. It simply does not matter, _as long as you can specify the start_number_.

The current code attempts to "guess" the pattern up front, based on the filename it's initially passed, and then scans the directory to see if it contains any other filenames that match that assumptive pattern. It's not just the fact that it's so inefficient and error-prone, though it is. (Part of the reason the feature sucks so hard, and makes me hate it so much.) But that's actually the harder approach, because it's completely bass-ackwards.

A far simpler approach is:

List bonsai

  1. Grab the list of filenames in the directory, sorted numerically (based on the trailing edge of each name, not counting the extension).
  2. Find the initially-selected filename's position on that list.
  3. Starting from that position, walk the list in both directions.
  4. At each step, check whether the next adjacent entry is the previous name with 1 either added or subtracted from it (again, at the least significant digit position), depending which direction.
  5. The first filename you encounter that isn't the previous name ±1, you've gone past end of the sequence. Discard that item, and all remaining items in that direction.
  6. Once you've determined the start and end filenames in the continuous list, and if the length of that list is >= 3, apply the prefix-trimming algorithm I laid out earlier to convert the names into any valid pattern with corresponding start_number.

Matching extensions

Oh, yeah, and one last wrinkle: The files in the sequence list all have to have the same extension as well. slide1_001.jpg, slide2_002.png, and slide3_003.jpg could never be an image sequence, since the non-uniform encoding would crash libavformat.

(In truth, to avoid crashing all of the files have to have exactly the same image properties: Same encoding, same pixel dimensions, same color depth, etc. Unfortunately, as I noted in #3044, OpenShot can't actually ensure they all match without opening each and every image file in the sequence, one by one. Which would be way too expensive an operation, for far too little reward.

mohamed-aiman commented 4 years ago

I faced this issue today. Just selected all files and renamed with Sequence and it worked. The output name format was.

Sequence(1).jpg Sequence(2).jpg Sequence(3).jpg

sg-github-account commented 4 years ago

I got this issue using openshot version 2.5.1 (libopenshot0.2.5) on Linux mint. File names were

DSCN0977.JPG DSCN0990.JPG DSCN1003.JPG DSCN1016.JPG DSCN1029.JPG DSCN1042.JPG DSCN1055.JPG DSCN1068.JPG DSCN1081.JPG DSCN1094.JPG DSCN1108.JPG DSCN1121.JPG DSCN0978.JPG DSCN0991.JPG DSCN1004.JPG DSCN1017.JPG DSCN1030.JPG DSCN1043.JPG DSCN1056.JPG DSCN1069.JPG DSCN1082.JPG DSCN1095.JPG DSCN1109.JPG DSCN1122.JPG DSCN0979.JPG DSCN0992.JPG DSCN1005.JPG DSCN1018.JPG DSCN1031.JPG DSCN1044.JPG DSCN1057.JPG DSCN1070.JPG DSCN1083.JPG DSCN1096.JPG DSCN1110.JPG DSCN0980.JPG DSCN0993.JPG DSCN1006.JPG DSCN1019.JPG DSCN1032.JPG DSCN1045.JPG DSCN1058.JPG DSCN1071.JPG DSCN1084.JPG DSCN1097.JPG DSCN1111.JPG DSCN0981.JPG DSCN0994.JPG DSCN1007.JPG DSCN1020.JPG DSCN1033.JPG DSCN1046.JPG DSCN1059.JPG DSCN1072.JPG DSCN1085.JPG DSCN1098.JPG DSCN1112.JPG

After a few hours I finally got it working by renaming / soft linking to

ln -s ../hail/DSCN0977.JPG 001.jpg ln -s ../hail/DSCN0978.JPG 002.jpg ln -s ../hail/DSCN0979.JPG 003.jpg ln -s ../hail/DSCN0980.JPG 004.jpg ln -s ../hail/DSCN0981.JPG 005.jpg ln -s ../hail/DSCN0982.JPG 006.jpg ln -s ../hail/DSCN0983.JPG 007.jpg ln -s ../hail/DSCN0984.JPG 008.jpg ln -s ../hail/DSCN0985.JPG 009.jpg

The code generation to rename all the files used awk and NR option

ls -1R | awk '{print "ln -s ../hail/"$0" 00"NR".jpg"}' >script1.txt

sg-github-account commented 4 years ago

With regards to the "The code generation to rename all the files used awk and NR option"

ls -1R | awk '{print "ln -s ../hail/"$0" 00"NR".jpg"}' >script1.txt

you will need to add a 0 to any first 9 files ie

ln -s ../hail/DSCN0977.JPG 001.jpg becomes ln -s ../hail/DSCN0977.JPG 0001.jpg

otherwise 0010.jpg+ will not be loaded.

stale[bot] commented 3 years ago

Thank you so much for submitting an issue to help improve OpenShot Video Editor. We are sorry about this, but this particular issue has gone unnoticed for quite some time. To help keep the OpenShot GitHub Issue Tracker organized and focused, we must ensure that every issue is correctly labelled and triaged, to get the proper attention.

This issue will be closed, as it meets the following criteria:

We'd like to ask you to help us out and determine whether this issue should be reopened.

Thanks again for your help!

gridge commented 2 years ago

I've just ran into the same issue. I've therefore downloaded the latest daily build (Version 2.6.1-dev, Build Date: 2022-05-26) and confirmed that I still see exactly the same problem.

I have a set of images that start from DSC_0326.JPG and continues for a few hundreds numbers up to DSC_1085.JPG. When I import the first file I get the prompt asking if I want to import the image as a sequence. When I answer yes I get the same dialog:

image

For completeness, note that I miss some pictures for some of the following numbers (the first one is that DSC_0336.JPG is missing), so that I'm not sure if this is now a simpler confusing error or if the same underlying problem as reported earlier is still there. I still wanted to report here.

NFGman commented 2 years ago

I was about to leave a fairly detailed note about how this is still very much an issue, because adding a single ABC_1234.jpg file by itself also caused the error.

But it's because adding one file in a folder full of extra similar files causes the 'do you want to import a sequence' dialogue to appear, and answering YES causes the error.

As a new user, adding a single file, I don't know if it's asking me if I'm creating a new sequence in the app, or if I want it to make the sequence from the images in the folder. So I just say 'yes, do the import, and why is this failing WTF'.

So, if I may be so bold:

  1. The dialogue isn't clear about what's happening
  2. The error isn't clear about what's failing

Maybe as a fix, my thinking is that if I drag one single file, I only want to import that. If I drag a folder or multiple files, maybe assume I want the sequence and only then ask to import a sequence?

seth586 commented 11 months ago

Still an issue on Openshot 3.1.1

images captured from my gopro are labeled as such:

G0020019.jpg G0020020.jpg G0020021.jpg

Colorjet3 commented 11 months ago

I've experienced this type of an issue with my GoPro as well. What I've done is to run them through a converter like VLC, Handbrake, or ShutterEncoder and convert them to .jpg files again. Import these new files and all is good.

junkbox666 commented 7 months ago

FWIW and as stated by ferdnyc it's important to ensure the file extensions are all the same. I'm using EbSynth to procedurally generate a png sequence off of a few stylized/painted keyframes from the original png sequence export (from OpenShot), then you dump the original sequence into EbSynth and the few painted Keyframes, EbSynth then makes its render but in separate folders depending on the keyframe number you chose...whatever, so I ended up with four folders which then rendered the sequence and the first ones imported (with the dialog) fine into OpenShot but I received the above error when importing a certain sequence...banging my head and googling the hell out of it I finally looked at the folder again and noticed I had saved the .afphoto (Affinity Photo) source file I was painting in the Keyframe folder, so while EbSynth didn't mind OpenShot did. So my boner move and me cursing the internet gods accomplished nothing towards solving the problem. Delete the rouge filetype and wow* the sequence imports fine. Granted the error message is a bit deceiving, the process isn't failing on the image stated in the error it's failing processing somewhere after that. In my case the sequence started at 280 and the duplicate and invalid filetype was 600. I don't expect the devs to PEBCAK proof it entirely, but maybe just a "hey dumbass, *600.afphoto is not a .png"

Cheers and thanks for all of your effort and hard work.

EDIT: ok so my logic may have been off, because the *.afphoto file was in the folder when EbSynth went to render the png's EbSynth may have glitched out or attempted to make a circle out of a square and produced an invalid png file which would have choked OpenShot's processing. Result is the same, as also stated above make sure each sequence is the same format, exactly...apparently.

-jb

Colorjet3 commented 7 months ago

Thank you @junkbox666 for the FYI. All I can tell you is that the importing of image sequence functionality needs a bit of TLC. I like your suggestion of at least make the error message (any error message generated by the image sequence importing) to be more precise so we can troubleshoot better.

Additionally, having good documentation to go with it that clearly defines what the current rules are would help to identify if something is a bug or some kind of an anomaly.

When time permits I will do some testing and document the results vs. what is expected to happen and may be a few of us can provide input and then we can generate an actual bug entry to have the lead developer take a look at.