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

Add support for videoconformancesink #74

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

ndufresne
Copy link
Contributor

@ndufresne ndufresne commented May 31, 2021

This PR is in preparation for GStreamer 1.20 and the new videoconformancesink being accepted. This is sent early, so this can get more attention and testing. The patch will check GStreamer version and use the conformance sink if GStreamer is new enough. With the new videoconformancesink, GStreamer padding is ignored, that padding was breaking some V8 and VP9 tests.

Pending GStremaer MR: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2287

@ndufresne ndufresne force-pushed the gstreamer-videoconformancesink branch 4 times, most recently from d2d5ccb to 3a6b99f Compare May 31, 2021 20:34
Copy link
Contributor

@pamarcos pamarcos left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR :)
LGTM. I added a little and fussy comment, though.

fluster/decoders/gstreamer.py Outdated Show resolved Hide resolved
@ndufresne ndufresne force-pushed the gstreamer-videoconformancesink branch 3 times, most recently from c536cc8 to 1eb97ab Compare June 3, 2021 16:24
@ndufresne ndufresne changed the title Draft: Add support for videoconformancesink Add support for videoconformancesink Jun 3, 2021
@ndufresne
Copy link
Contributor Author

Ok, I believe I fixed the first round of review. We settle to videocodectestsink upstream, so I updated the code accordingly also.

Copy link
Contributor

@pamarcos pamarcos left a comment

Choose a reason for hiding this comment

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

LGTM, apart of the fussy comment. Nice work 👍
The gst_element_exists you implemented with lru_cache is exactly what I meant

fluster/decoders/gstreamer.py Show resolved Hide resolved
When available, use the new videocodectestsink instead of filesink. This
new sink will produced the appropriate YUV data needed.
@ndufresne ndufresne force-pushed the gstreamer-videoconformancesink branch from 1eb97ab to efc3e43 Compare June 7, 2021 19:49
@ndufresne
Copy link
Contributor Author

@pamarcos ok, added the "safety" return, and simplified the commit message a bit with only what's relevant to know from a testing point of view.

@pamarcos pamarcos merged commit 253f95c into fluendo:master Jun 8, 2021
@pamarcos
Copy link
Contributor

pamarcos commented Jun 8, 2021

Perfect @ndufresne! Thanks a mill for your contribution 💪

@ndufresne
Copy link
Contributor Author

My next goal is to setup upstream GStreamer CI for software VP8 decode, as this one is passing, I should be able to setup CI for VP9 as well after figuring the one remaining issue, for H.264/H.265, we'll need to think of a clean way to mark as known to fail some of the tests, as it seems we'll never fully pass these, and for HW, we have to deal with HW errors, so such system could be reuse, we basically want to catch regression (or improvement).

@pamarcos
Copy link
Contributor

pamarcos commented Jun 9, 2021

My next goal is to setup upstream GStreamer CI for software VP8 decode, as this one is passing, I should be able to setup CI for VP9 as well after figuring the one remaining issue, for H.264/H.265, we'll need to think of a clean way to mark as known to fail some of the tests, as it seems we'll never fully pass these, and for HW, we have to deal with HW errors, so such system could be reuse, we basically want to catch regression (or improvement).

What we use for our CI are the --threshold to ensure a minimum amount of tests and the --time-threshold to ensure no performance regressions. Finally, we have stored known fluster results for specifc nodes in a repo. Then, our CI checks that there is no diff with them, except for known racy/fluke tests. Something like:

// Check for differences, ignoring extra blank lines, CRLF line endings and the test name,
// because we reuse the same output for different decoders in H.265
def diffParams = '-uB -I"|Test|" --strip-trailing-cr'
diffParams += ' -I"|FM1"' // TODO: remove once OCP-1853 is fixed
sh "diff $diffParams expected.out ${this.repoDir}/summary.log"

The expected.out taken from the repo is the output of fluster for that particular decoder when generating the summary. When comparing, we only take into account changes in the result of tests in the summary. This has helped us so far to achieve stability, catching regressions earlier in the dev process.

@ndufresne
Copy link
Contributor Author

This is gold, thanks.

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.

None yet

3 participants