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

[FEATURE] SCC and CCD encoder #1154

Merged
merged 28 commits into from Jan 18, 2020
Merged

[FEATURE] SCC and CCD encoder #1154

merged 28 commits into from Jan 18, 2020

Conversation

@NilsIrl
Copy link
Contributor

NilsIrl commented Dec 22, 2019

Fix #1120

  • 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.

Colours haven't been implemented yet.

The disassembly format hasn't been implemented yet.

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 22, 2019

The bitmap OCR hasn't been implemented either

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 22, 2019

Dealing with different line endings hasn't been implemented yet either ("\n" vs "\r\n")

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 22, 2019

Modifying subtitles hasn't been implemented yet. As mentioned in #1139 (comment) it's probably better if it was done in a different way

@NilsIrl NilsIrl marked this pull request as ready for review Dec 22, 2019
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 22, 2019

I will need more examples to be able to continue with the rest

@NilsIrl NilsIrl force-pushed the NilsIrl:ssc_encoder branch from 1d90ed5 to 44dc86f Dec 22, 2019
@NilsIrl NilsIrl changed the title [FEATURE] Ssc encoder [FEATURE] SCC and CCD encoder Dec 22, 2019
src/lib_ccx/ccx_encoders_srt.c Outdated Show resolved Hide resolved
src/lib_ccx/ccx_common_common.c Outdated Show resolved Hide resolved
src/lib_ccx/ccx_encoders_helpers.c Outdated Show resolved Hide resolved
src/lib_ccx/ccx_encoders_scc.c Outdated Show resolved Hide resolved
src/lib_ccx/utility.c Show resolved Hide resolved
src/lib_ccx/ccx_encoders_scc.c Outdated Show resolved Hide resolved
@NilsIrl NilsIrl force-pushed the NilsIrl:ssc_encoder branch from f1dca33 to 4cb1e36 Dec 23, 2019
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 23, 2019

For some reason the .srt output doesn't work anymore

@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Dec 23, 2019

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 23, 2019

Because?

As I said "some reason". 🤣

I have no clue. I thought for some time it had to do with using enums instead of ints but that's not the problem. Also webvtt isn't affected so....

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 23, 2019

Okay, fixed it

I didn't notice the if statement after that which meant these values still had to be initialized

@NilsIrl NilsIrl force-pushed the NilsIrl:ssc_encoder branch from 6df6396 to ea49e5a Dec 23, 2019
Copy link
Contributor

thealphadollar left a comment

Amazing work @NilsIrl

@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Dec 25, 2019

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

Report Name Tests Passed
Broken 0/13
DVB 0/7
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Teletext 0/21
WTV 0/13
XDS 0/34
CEA-708 0/14
DVD 0/3
Options 1/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.
Copy link
Contributor

cfsmp3 left a comment

This could be reasonably be merged. But it's missing an entry on the change log and, I assume, help screen.

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 26, 2019

Here is a list of post merge operations to perform:

  • Unify the subtitle modifying features (Capitalization + Bad word filtration)
  • Unify dealing with OCR stuff
  • Unifying addition of delay which will fix #1103
  • Change NixOS build system to not use zlib
@NilsIrl NilsIrl force-pushed the NilsIrl:ssc_encoder branch from 74346dd to 70f8cce Dec 26, 2019
@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Dec 29, 2019

Still fails:

1>ccx_encoders_scc.obj : error LNK2019: unresolved external symbol _dprintf referenced in function _add_padding
1>Debug\ccextractorwin.exe : fatal error LNK1120: 1 unresolved externals
1>Done building project "ccextractor.vcxproj" -- FAILED.

Just get rid of that dprintf() :-)

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 29, 2019

Just get rid of that dprintf() :-)

Yeah I realised MSVC actually has a dprintf but it's a totally different function

@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Dec 29, 2019

Just get rid of that dprintf() :-)

Yeah I realised MSVC actually has a dprintf but it's a totally different function

In general (not just for us) - submitting untested stuff decreases your street cred :-) It's OK we find bugs when we test corner cases, but a PR that doesn't even compile is not good...

We want CCExtractor to be portable (as is it builds for anything that has a C compiler), so in general, any function that is an extension of a specific compiler (even if it is GCC) is best not to use.

@NilsIrl NilsIrl force-pushed the NilsIrl:ssc_encoder branch from 1d1f35d to d8211d1 Dec 31, 2019
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 31, 2019

I changed the architecture to make it less "hacky". That is, this new architecture, is more linear and doesn't try to make sense of what is seemingly random. Codes are now hardcoded. This will make it easier to add features (color) without having to make it more complex.

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Dec 31, 2019

I'll also add that this last commit gives the exact same output as the previous one. (except that it doesn't produce trailing space anymore)

@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Dec 31, 2019

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

Report Name Tests Passed
Broken 0/13
DVB 0/7
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Teletext 0/21
WTV 0/13
XDS 0/34
CEA-708 0/14
DVD 0/3
Options 0/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 6, 2020

@NilsIrl can you resolve conflicts? (and let us know the current status)

@NilsIrl NilsIrl force-pushed the NilsIrl:ssc_encoder branch from d8211d1 to 54c6b5a Jan 6, 2020
@NilsIrl NilsIrl force-pushed the NilsIrl:ssc_encoder branch from 9ded82e to 39b99e2 Jan 16, 2020
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 18, 2020

Another thing that isn't implemented is preamble codes that set the font and the placement on the screen at the same time.

For the moment, it uses 2 codes for that (1 for the placement and 1 for the font). This isn't really problematic because it is rare the column is one that can be set using these preamble codes.

@cfsmp3 cfsmp3 merged commit 1924174 into CCExtractor:master Jan 18, 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:ssc_encoder branch Jan 21, 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.