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

test/libmpv_test: add default track selection testing #14393

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dudemanguy
Copy link
Member

The amount of options we have that are related track selection is insane and touching any of this stuff is scary. Add some track selection testing to the CI to hopefully make it slightly less scary to touch any of that. Since trying to work in media files from some outside source would be a pain and need to live in some repo somewhere, it's nicer to just generate dummy files on the fly with ffmpeg during the ci runs and use that. Obviously, the actual tests themselves could be even more thorough (external tracks were not even considered), but this is more than a good enough start for now.

Read this before you submit this pull request:
https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md

Reading this link and following the rules will get your pull request reviewed
and merged faster. Nobody wants lazy pull requests.

@Dudemanguy Dudemanguy force-pushed the test-default-track-selection branch 4 times, most recently from d9d5e5c to cad0996 Compare June 20, 2024 04:35
Copy link

github-actions bot commented Jun 20, 2024

Download the artifacts for this pull request:

Windows
macOS

@Dudemanguy
Copy link
Member Author

Dudemanguy commented Jun 20, 2024

Just for the record, I excluded the mingw, win32 (laziness) , openbsd (the ffmpeg commands got stuck), and ffmpeg 4.4 (the ffmpeg commands are bugged and give wrong output) from running the track selection tests.

Edit: dated.


# Create core video, audio, and subtitle streams to construct files out of.
ffmpeg -y -f lavfi -i testsrc=duration=2:size=1280x720 video.mkv
ffmpeg -y -f lavfi -i sine=frequency=1000:duration=2 audio.flac
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be better to do it with mpv?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would personally prefer avoiding mpv's encoding mode.

@Dudemanguy Dudemanguy force-pushed the test-default-track-selection branch 2 times, most recently from 435b1c8 to 0a8dda2 Compare June 20, 2024 14:40
test/meson.build Outdated Show resolved Hide resolved
ffmpeg = find_program('ffmpeg', required: false)
if ffmpeg.found() and ffmpeg.version().version_compare('>=0.5')
generate_test_media = find_program(join_paths(source_root, 'ci', 'generate-test-media.sh'), required: true)
run_command(generate_test_media, build_root, check: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use custom_target to generate it on demand and not on every configure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, custom_target is really annoying in this context.

Copy link
Contributor

@kasper93 kasper93 Jun 22, 2024

Choose a reason for hiding this comment

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

I have not tried myself. But wouldn't it be

test_files = []
test_file += custom_target('eng_default.mkv',
    output: 'eng_default.mkv',
    command: [ffmpeg, ..., '@OUTPUT@'],
)

and then

test('libmpv', exe, args: [file, test_files], timeout: 60, should_fail: not features['lua'])

remove

const char *files[]

array and ready input files from argv.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generating files on every configure unconditionally with external ffmpeg is no-go for me. Exact solution is up in the air, but I would prefer it not do complex stuff in configure.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would work but you would need a different target for every single generated file which feels kind of stupid. But I haven't thought of anything else better yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, for me it is natural, they are different commads after all

@Dudemanguy Dudemanguy force-pushed the test-default-track-selection branch from 0a8dda2 to 9feae32 Compare June 22, 2024 14:17
The amount of options we have that are related track selection is insane
and touching any of this stuff is scary. Add some track selection
testing to the CI to hopefully make it slightly less scary to touch any
of that. Since trying to work in media files from some outside source
would be a pain and need to live in some repo somewhere, it's nicer to
just generate dummy files on the fly with ffmpeg during the ci runs and
use that. Obviously, the actual tests themselves could be even more
thorough (external tracks were not even considered), but this is more
than a good enough start for now.
@Dudemanguy Dudemanguy force-pushed the test-default-track-selection branch from 9feae32 to 7e70efe Compare June 22, 2024 14:20
@@ -271,9 +288,140 @@ static void test_options_and_properties(void)
fail("Node: expected 1 but got %d'!\n", result_node.u.flag);
}

static void test_track_selection(char *directory)
Copy link
Member

Choose a reason for hiding this comment

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

how about moving splitting this into a different file and test run?

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that but this file has a lot of generic useful stuff in it for the test.

Copy link
Member

Choose a reason for hiding this comment

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

I would move them to a shared file.

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