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

Unit tests fail (spectacularly) on every arch except i386/x86_64 #332

Closed
ferdnyc opened this issue Sep 19, 2019 · 1 comment · Fixed by #339
Closed

Unit tests fail (spectacularly) on every arch except i386/x86_64 #332

ferdnyc opened this issue Sep 19, 2019 · 1 comment · Fixed by #339
Labels
build Issues related to compiling or installing libopenshot and its dependencies code Source code cleanup, streamlining, or style tweaks pending A PR has been submitted to fix the issue Readers: media Issues with the readers that import content from external formats (video, audio, images)

Comments

@ferdnyc
Copy link
Contributor

ferdnyc commented Sep 19, 2019

This is effectively a dupe of #51 from two years ago, but since (a) that was two years ago, and (b) I have much more copious logs, which (c) are from the current version, I'm opening as a new issue. (I'll probably close #51 as outdated. Also related: #43, #69, #65)

After finally solving the Python discovery issue on Fedora Rawhide (#331), I was finally ready to attempt new builds of libopenshot. Along the way, I decided to flip on the unit tests, which up until now we haven't been running.

They failed, fairly spectacularly, on every arch other than i386 / x86_64. While previous reports from the Debian team highlighted 3 or 4 failures in the suite, at this point we're up to twenty-one failures on both armv7hl and aarch64. (Our ppc64le builds failed in the compile step, though it's possible that's an environmental issue on the RPM Fusion builders.)

And that's not even counting the copious warning messages ffmpeg spewed throughout the process, primarily about unaccelerated colorspace conversions. So, before I turn the tests back off so I can complete my builds, I wanted to document the failures here.

They're found in the ImageWriter, FFmpegReader, and Timeline test modules, and all consist of surprisingly-identical off-by-one or off-by-two value errors. (The biggest discrepancy is the first ImageWriter test, which produces 25 when the expected value is 20):

// Get the image data for row 500
const unsigned char* pixels = f->GetPixels(500);
int pixel_index = 230 * 4; // pixel 230 (4 bytes per pixel)
// Check image properties
CHECK_EQUAL(20, (int)pixels[pixel_index]);
CHECK_EQUAL(18, (int)pixels[pixel_index + 1]);
CHECK_EQUAL(11, (int)pixels[pixel_index + 2]);
CHECK_EQUAL(255, (int)pixels[pixel_index + 3]);

Now, I just discovered from reading the code that the ImageWriter test is using a GIF image as the test format, which strikes me as a spectacularly dicey idea. (It's easy to say that now, when faced with failing tests, of course.)

But the FFmpegReader test is failing in direct reads from test.mp4. Now, it's only failing by small degrees in the middle ground — like, in the four checks here, the latter two pass, and the first two come back with results of 22 and 193 which is barely off.

// Check image properties on scanline 10, pixel 112
CHECK_EQUAL(21, (int)pixels[pixel_index]);
CHECK_EQUAL(191, (int)pixels[pixel_index + 1]);
CHECK_EQUAL(0, (int)pixels[pixel_index + 2]);
CHECK_EQUAL(255, (int)pixels[pixel_index + 3]);
// Check pixel function
CHECK_EQUAL(true, f->CheckPixel(10, 112, 21, 191, 0, 255, 5));
CHECK_EQUAL(false, f->CheckPixel(10, 112, 0, 0, 0, 0, 5));

(Aside: With a threshold having been added to CheckPixel() — the reason those last tests pass, when the prior ones don't — wouldn't it make sense to use CHECK_CLOSE() for the direct tests, with that same threshold? Otherwise... well, you end up with the situation we're currently in.)

Regardless, this bears looking at, because failing tests on non-Intel platforms may point to issues that we're merely failing to catch on Intel platforms, rather than non-issues. Or, if it's merely that thresholds need adjusting, that's fine too. But passing tests even on non-target platforms is a worthwhile goal.

Full build logs attached.

aarch64 on Fedora 30: aarch64-F30.log

armv7 on (future-)Fedora 31: armv7-F31.log

I was actually incorrect, the test failures aren't the same on both platforms. (I was looking at the aarch64 logs for both Fedora 30 and Fedora 31, which less-surprisingly are basically identical.) The armv7 and aarch64 tests both report 21 failures from the 82 tests in the suite, but interestingly enough they're not the same 21 failures. So, that may be informative.

@ferdnyc ferdnyc added Readers: media Issues with the readers that import content from external formats (video, audio, images) code Source code cleanup, streamlining, or style tweaks build Issues related to compiling or installing libopenshot and its dependencies labels Sep 19, 2019
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Sep 19, 2019

Build jobs in the RPM Fusion Koji, for all supported architectures:

@ferdnyc ferdnyc added the pending A PR has been submitted to fix the issue label Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to compiling or installing libopenshot and its dependencies code Source code cleanup, streamlining, or style tweaks pending A PR has been submitted to fix the issue Readers: media Issues with the readers that import content from external formats (video, audio, images)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant