Some issues with large files #3293

Merged
merged 3 commits into from Feb 9, 2017

Conversation

Projects
None yet
2 participants
@zonkmachine
Member

zonkmachine commented Jan 25, 2017

In response to two related crashes in SampleBuffer.cpp as commented in #3205
I substituted the command line messages for a message box.
I haven't touched the original 100 MB size limit and have added a 1 hour length check.


FIx explained:
I add a bool, fileLoadError, which is set to false, should the file fail on one of the tests. I add a test for file length as a compressed file of small size when decompressed will take a much bigger size so the important fact to know for LMMS is not the file size but the length of the sample. This operation takes a bit of time to carry out compared to the file size test so we wait to see if that one passes and then carry on testing the length of the sample.
If the file is marked fileLoadError == true, the buffer will be written with one sample, value 0, and after removing possible locks a message is shown. The message is only for information and is shown after the locks are removed so the project isn't frozen while waiting for 'OK' to be pressed. The old message was to the command line and of little use to an ordinary user.

Some issues that remain with opening large files:

  • Previewing large files is sluggish. The file tests works however but large file within the limit are still loaded in their entirety. If you open large files you do it from the track container rather than the menu.
  • After failing to load a file on being to large the buffer is zeored and you might want to keep the old file in case of a fail. In order to fix this you (probably) need to test the file earlier, before calling SampleBuffer::update() .

@zonkmachine zonkmachine added the bug label Jan 25, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jan 29, 2017

Member

This is ready for review.

Member

zonkmachine commented Jan 29, 2017

This is ready for review.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jan 30, 2017

Member

I moved the message box to the end as it would interrupt the song playing. First take care of everything and remove lock, then disappoint the user...

Me thinks SampleBuffer::update( ... ) is trying to do a bit too much. The file checks could be done in a separate function, and in advance of updating it, and then we can avoid writing blank, 1 sample buffers, on loading a new sample that fails. I think it's more useful to keep the old one in case this happens.

Member

zonkmachine commented Jan 30, 2017

I moved the message box to the end as it would interrupt the song playing. First take care of everything and remove lock, then disappoint the user...

Me thinks SampleBuffer::update( ... ) is trying to do a bit too much. The file checks could be done in a separate function, and in advance of updating it, and then we can avoid writing blank, 1 sample buffers, on loading a new sample that fails. I think it's more useful to keep the old one in case this happens.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 8, 2017

Member

I've updated the first post with an explanation of the fix. I think this is a reasonable fix for now.

Member

zonkmachine commented Feb 8, 2017

I've updated the first post with an explanation of the fix. I think this is a reasonable fix for now.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Feb 9, 2017

Member

I've updated the first post with an explanation of the fix. I think this is a reasonable fix for now.

You've added a QMessageBox to a non-gui class, so I still think we need to check for a GUI via if(gui) ... despite the near-impossible likelihood of a command-line render with a 60 minute sample.

Member

tresf commented Feb 9, 2017

I've updated the first post with an explanation of the fix. I think this is a reasonable fix for now.

You've added a QMessageBox to a non-gui class, so I still think we need to check for a GUI via if(gui) ... despite the near-impossible likelihood of a command-line render with a 60 minute sample.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 9, 2017

Member

OK, fixed that. Stumbled upon #3338 so I had to backport to 6137fcc to test this.

$ ./lmms -r ~/lmms/projects/buggar/largeSamples/90min.mmp -o test.wav
...
Loading project...
Audio files are limited to 100 MB in size and 1 hour of playing time
$
Member

zonkmachine commented Feb 9, 2017

OK, fixed that. Stumbled upon #3338 so I had to backport to 6137fcc to test this.

$ ./lmms -r ~/lmms/projects/buggar/largeSamples/90min.mmp -o test.wav
...
Loading project...
Audio files are limited to 100 MB in size and 1 hour of playing time
$

@tresf tresf merged commit a0caecc into LMMS:master Feb 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zonkmachine zonkmachine deleted the zonkmachine:largefiles branch Feb 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment