Skip to content

[FEATURE] SCC and CCD encoder#1154

Merged
cfsmp3 merged 28 commits intoCCExtractor:masterfrom
NilsIrl:ssc_encoder
Jan 18, 2020
Merged

[FEATURE] SCC and CCD encoder#1154
cfsmp3 merged 28 commits intoCCExtractor:masterfrom
NilsIrl:ssc_encoder

Conversation

@NilsIrl
Copy link
Copy Markdown
Contributor

@NilsIrl NilsIrl commented Dec 22, 2019 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

NilsIrl commented Dec 22, 2019

The bitmap OCR hasn't been implemented either

@NilsIrl
Copy link
Copy Markdown
Contributor Author

NilsIrl commented Dec 22, 2019

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

@NilsIrl
Copy link
Copy Markdown
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 December 22, 2019 14:48
@NilsIrl
Copy link
Copy Markdown
Contributor Author

NilsIrl commented Dec 22, 2019

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

@NilsIrl NilsIrl changed the title [FEATURE] Ssc encoder [FEATURE] SCC and CCD encoder Dec 22, 2019
cfsmp3
cfsmp3 requested changes Dec 22, 2019
View reviewed changes
Comment thread src/lib_ccx/ccx_encoders_srt.c Outdated
Comment thread src/lib_ccx/ccx_common_common.c Outdated
Comment thread src/lib_ccx/ccx_encoders_helpers.c Outdated
Comment thread src/lib_ccx/ccx_encoders_scc.c Outdated
Comment thread src/lib_ccx/utility.c
NilsIrl
NilsIrl commented Dec 23, 2019
View reviewed changes
Comment thread src/lib_ccx/ccx_encoders_scc.c Outdated
NilsIrl
NilsIrl commented Dec 23, 2019
View reviewed changes
Comment thread src/lib_ccx/ccx_common_constants.h Outdated
@NilsIrl
Copy link
Copy Markdown
Contributor Author

NilsIrl commented Dec 23, 2019

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

@cfsmp3
Copy link
Copy Markdown
Contributor

cfsmp3 commented Dec 23, 2019 via email

@NilsIrl
Copy link
Copy Markdown
Contributor Author

NilsIrl commented Dec 23, 2019 •
edited
Loading

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
Copy link
Copy Markdown
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

thealphadollar
thealphadollar reviewed Dec 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

Amazing work @NilsIrl

@ccextractor-bot
Copy link
Copy Markdown
Collaborator

ccextractor-bot commented Dec 25, 2019 •
edited
Loading

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.

cfsmp3
cfsmp3 requested changes Dec 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@cfsmp3 cfsmp3 left a comment

Choose a reason for hiding this comment

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

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

@NilsIrl
Copy link
Copy Markdown
Contributor Author

NilsIrl commented Dec 26, 2019 •
edited
Loading

Here is a list of post merge operations to perform:

@cfsmp3
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Collaborator

ccextractor-bot commented Dec 31, 2019 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

cfsmp3 commented Jan 6, 2020

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

NilsIrl added 24 commits January 16, 2020 21:33
@NilsIrl
Copy link
Copy Markdown
Contributor Author

NilsIrl commented Jan 18, 2020 •
edited
Loading

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
This was referenced Jan 18, 2020
Closed
Closed
@NilsIrl NilsIrl deleted the ssc_encoder branch January 21, 2020 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cfsmp3 cfsmp3 cfsmp3 requested changes

+1 more reviewer

@thealphadollar thealphadollar thealphadollar left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[PROPOSAL] Add an option to output Scenarist Closed Caption format (*.SCC) and/or McPoodle's Disassembly format (*.CCD)

4 participants

@NilsIrl @cfsmp3 @ccextractor-bot @thealphadollar