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 Chapter Support #49

Merged
merged 4 commits into from
May 31, 2024
Merged

Add Chapter Support #49

merged 4 commits into from
May 31, 2024

Conversation

Arthi-chaud
Copy link
Contributor

Hello!

This PR is a contribution similar to #45, with the fixes you requested in the PR.

Regarding testing, I added chapters to the test.mp4 file and added/updated some tests.

Please let me know if something else is needed.

Copy link
Owner

@vansante vansante left a comment

Choose a reason for hiding this comment

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

Some comments

ffprobe_test.go Outdated Show resolved Hide resolved
probedata.go Outdated Show resolved Hide resolved
probedata.go Outdated Show resolved Hide resolved
probedata.go Outdated Show resolved Hide resolved
probedata.go Outdated Show resolved Hide resolved
@Arthi-chaud Arthi-chaud requested a review from vansante May 28, 2024 15:44
@Arthi-chaud
Copy link
Contributor Author

Everything's been fixed! :)

Copy link
Owner

@vansante vansante left a comment

Choose a reason for hiding this comment

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

I have a few more nitpicks

probedata.go Outdated Show resolved Hide resolved
probedata.go Show resolved Hide resolved
@Arthi-chaud
Copy link
Contributor Author

Should I do something about the lint errors? (since it's caused by the test)

Also, not sure why the test workflow fails (Tested in a dockerised environment, with the same setup as the workflow). Help would be appreciated. :)

@Arthi-chaud Arthi-chaud requested a review from vansante May 29, 2024 16:05
@vansante
Copy link
Owner

The linter fails because the validateData() function is now too long(=complex), which means the best course of action is to split it up in validateStreams(), validateChapter(), etc.

As for the test failure, its fails on:

ffprobe_test.go:143: It does not have a subtitle stream.
ffprobe_test.go:149: It does not have a data stream.

The second one you adjusted in your test update, was that needed?
As for the first one, I have honestly no clue, will look into it later.

@Arthi-chaud
Copy link
Contributor Author

I split the tests into smaller functions.

It looks like the mp4 containers' handling of chapters adds a stream which, depending on the version of ffprobe, can be interpreted as a subtitle stream (posts about it here and here)
On my machine (macOS, M1), I do not have the issue. My guess is that it is related to ffmpeg itself.

@vansante
Copy link
Owner

Let me see if I can upgrade some things (like ffprobe) to fix it.

@vansante
Copy link
Owner

Allright, I have upgraded the pipelines to run the latest release version of ffprobe, however, I did not notice any difference in tests:

#50

@vansante
Copy link
Owner

I guess I would suggest to cater the test results to what we expect on an ubuntu machine.

@Arthi-chaud
Copy link
Contributor Author

Hi! Just rebased the fork so that it would use the updated GH actions.

Looks like the 2nd run of the test did not use the latest version of the tests. Would you mind re-running time just to check?

Copy link
Owner

@vansante vansante left a comment

Choose a reason for hiding this comment

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

Looks good, lets merge it!

@vansante vansante merged commit e5dd79a into vansante:v2 May 31, 2024
2 checks passed
@vansante
Copy link
Owner

It was time for a new release, so here it is

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.

2 participants