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 Stabilize_Video test for platforms that doesn't use fast color space conversion #929

Merged

Conversation

kxxt
Copy link
Contributor

@kxxt kxxt commented May 28, 2023

The Stabilize_Video test fails on platforms that doesn't support accelerated colorspace conversion from yuv420p to rgba.

See the ffmpeg code for more info: https://github.com/FFmpeg/FFmpeg/blob/e8e486332571347dd55822c842ba67276ac308e2/libswscale/yuv2rgb.c#L678-L696

This PR fixes it by rounding the result that caused the test failure.

On x86_64, y = -0.001074643496478603
On riscv64, y = -0.0009904620157904118

The difference mainly comes from sws_scale function in src/FFmpegReader.cpp. (FMA instructions in OpenCV's library caused some insignificant difference that we can ignore here)

The following patch can be used to save a sample of the resulting buffers:

diff --git a/src/FFmpegReader.cpp b/src/FFmpegReader.cpp
index 291f585c..eba954f0 100644
--- a/src/FFmpegReader.cpp
+++ b/src/FFmpegReader.cpp
@@ -16,6 +16,9 @@
 #include <thread>	// for std::this_thread::sleep_for
 #include <chrono>	// for std::chrono::milliseconds
 #include <unistd.h>
+#include <iostream>
+#include <fstream>
+using namespace std;
 
 #include "FFmpegUtilities.h"
 
@@ -1485,10 +1488,18 @@ void FFmpegReader::ProcessVideoPacket(int64_t requested_frame) {
 	SwsContext *img_convert_ctx = sws_getContext(info.width, info.height, AV_GET_CODEC_PIXEL_FORMAT(pStream, pCodecCtx), width,
 												 height, PIX_FMT_RGBA, scale_mode, NULL, NULL, NULL);
 
+	cerr << "Before sws_scale" << endl;
+	auto file = std::fstream("1st.before.bin", std::ios::out | std::ios::binary);
+	file.write((const char *)pFrame->data[0], pFrame->linesize[0]);
+	file.close();
 	// Resize / Convert to RGB
 	sws_scale(img_convert_ctx, pFrame->data, pFrame->linesize, 0,
 			  original_height, pFrameRGB->data, pFrameRGB->linesize);
-
+	cerr << "After sws_scale" << endl;
+	file = std::fstream("1st.after.bin", std::ios::out | std::ios::binary);
+	file.write((const char *)pFrameRGB->data[0], pFrameRGB->linesize[0]);
+	file.close();
+	abort();
 	// Create or get the existing frame object
 	std::shared_ptr<Frame> f = CreateFrame(current_frame);
 

On x86_64:

$ sha256sum build/tests/1st.before.bin 
7685abd4a249adefa3fdcb5307cb3eb987d3e28294d7c2e4b82daee2580c7450  build/tests/1st.before.bin

$ sha256sum build/tests/1st.after.bin 
4e29bc67f3aabcfdb8f23228e4195efcea9afd778520989a8713d957d0358d55  build/tests/1st.after.bin

On riscv64:

[root@kxxt2 src]# sha256sum build/tests/1st.before.bin
7685abd4a249adefa3fdcb5307cb3eb987d3e28294d7c2e4b82daee2580c7450  build/tests/1st.before.bin
[root@kxxt2 src]# sha256sum build/tests/1st.after.bin
0d3a7149c8785610ad1e6ecaa1431bf589fac4ff6b6809e773cc6093735f73dc  build/tests/1st.after.bin

Close #893

…ace conversion

The Stabilize_Video test fails on platforms that doesn't support
accelerated colorspace conversion from yuv420p to rgba.

https://github.com/FFmpeg/FFmpeg/blob/e8e486332571347dd55822c842ba67276ac308e2/libswscale/yuv2rgb.c#L678-L696

This PR fixes it by rounding the result that caused the test failure.

On x86_64, y = -0.001074643496478603
On riscv64, y = -0.0009904620157904118
@XieJiSS
Copy link

XieJiSS commented May 30, 2023

The mentioned FMA behavior is also reproducible on x86_64 if compiled with -mfma flag.

kxxt added a commit to kxxt/archriscv-packages that referenced this pull request May 30, 2023
felixonmars pushed a commit to felixonmars/archriscv-packages that referenced this pull request May 31, 2023
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #929 (e6f6b64) into develop (e91bd82) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #929   +/-   ##
========================================
  Coverage    53.66%   53.66%           
========================================
  Files          182      182           
  Lines        16829    16829           
========================================
  Hits          9031     9031           
  Misses        7798     7798           
Impacted Files Coverage Δ
tests/CVStabilizer.cpp 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jonoomph
Copy link
Member

jonoomph commented Jun 2, 2023

Thanks! LGTM

@jonoomph jonoomph merged commit 887b993 into OpenShot:develop Jun 2, 2023
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.

CVStabilizer:Stabilize_Video failed on riscv64
3 participants