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] Make -delay all output formats #1167

Merged
merged 4 commits into from Jan 10, 2020
Merged

[Fix] Make -delay all output formats #1167

merged 4 commits into from Jan 10, 2020

Conversation

@NilsIrl
Copy link
Contributor

NilsIrl commented Jan 1, 2020

Fix #1103

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.
  • I have mentioned this change in the changelog.

My familiarity with the project is as follows (check one):

  • I have used CCExtractor just a couple of times.

Move up the pipeline, when the calculation of delay is done.

@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Jan 1, 2020

CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results:

Report Name Tests Passed
Broken 12/13
DVB 3/7
DVR-MS 2/2
General 27/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Teletext 14/21
WTV 13/13
XDS 33/34
CEA-708 14/14
DVD 3/3
Options 85/86

It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you).

Your PR breaks these cases:


Check the result page for more info.
@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Jan 1, 2020

CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results:

Report Name Tests Passed
Broken 12/13
DVB 4/7
DVR-MS 2/2
General 27/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Teletext 21/21
WTV 13/13
XDS 33/34
CEA-708 14/14
DVD 3/3
Options 84/86

It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you).

Your PR breaks these cases:


Check the result page for more info.
@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 1, 2020

How have you tested this? @NilsIrl
You seem to have good ideas and know what you are doing but lack in the testing side of things :-) That's also really important, our CI system is still not perfect and the rest of us also like to be coding rather than testing...

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 1, 2020

@cfsmp3 I've tried using with and without -delay 1000 with srt, webvtt, and snupng using the file and arguments of #1166 (except -out=webvtt which changes).

Also please note that the end_time was already calculated earlier in some cases and that there were more than one encoder which didn't take into account subs_delay so this fixed more than webvtt.

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 1, 2020

I also tested with FX.ts with ssa, srt and webvtt

@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 1, 2020

@canihavesomecoffee OK to merge? I can't test myself for the next 48 hours

@canihavesomecoffee

This comment has been minimized.

Copy link
Member

canihavesomecoffee commented Jan 1, 2020

@canihavesomecoffee OK to merge? I can't test myself for the next 48 hours

Will check tomorrow

@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 6, 2020

@NilsIrl Can you solve the conflicts?

@NilsIrl NilsIrl force-pushed the NilsIrl:#1103 branch from 92e2700 to e872e14 Jan 6, 2020
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 6, 2020

Conflict fixed

@canihavesomecoffee

This comment has been minimized.

Copy link
Member

canihavesomecoffee commented Jan 6, 2020

Linux tests look OK to me, Windows is still running.

@cfsmp3 cfsmp3 self-assigned this Jan 8, 2020
@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 8, 2020

Windows fails with error code 10 on some samples, which means NO_CAPTIONS. But it's producing that even when there's actually subtitles.

Note though that we return "no captions" for any error returned by the main loop (which sucks - the error can be anything else and we just happily convert it to no captions).

Don't know why this would appear in Win and not linux but clearly there's something weird going on here.

@NilsIrl can you check that out, see if you can figure it out?

@cfsmp3 cfsmp3 assigned NilsIrl and unassigned cfsmp3 Jan 8, 2020
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 9, 2020

Windows fails with error code 10 on some samples, which means NO_CAPTIONS. But it's producing that even when there's actually subtitles.

Is this a regression that was introduced in this PR? Because comparing with the results from master, I can't see a difference.

@canihavesomecoffee

This comment has been minimized.

Copy link
Member

canihavesomecoffee commented Jan 9, 2020

You are right @NilsIrl. I looked back a bit further, and at least since the release from 0.88 they have been returning non-expected return codes.

As such, this PR doesn't make it worse, and can be merged 👍

@cfsmp3 cfsmp3 merged commit 34d0df1 into CCExtractor:master Jan 10, 2020
2 of 4 checks passed
2 of 4 checks passed
CI - linux Not all tests completed successfully, please check
Details
CI - windows Not all tests completed successfully, please check
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@NilsIrl NilsIrl deleted the NilsIrl:#1103 branch Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.