Skip to content

Conversation

@jshin313
Copy link
Contributor

@jshin313 jshin313 commented Dec 5, 2019

Please prefix your pull request with one of the following: [FEATURE] [FIX] [IMPROVEMENT].

In raising this pull request, I confirm the following (please check boxes):

  • 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 never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

The pull request addresses the issue linked here as part of a Google Code-in task. Thus, expect less than optimal code from a high school student. The changes were tested on a Windows 10 machine using Visual Studio 2019 to build ccextractor.

@ccextractor-bot
Copy link
Collaborator

ccextractor-bot commented Dec 5, 2019

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

Report Name Tests Passed
Broken 13/13
DVB 5/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 81/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
Copy link
Collaborator

ccextractor-bot commented Dec 5, 2019

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

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

  • ccextractor -autoprogram -out=srt -latin1 85271be4d2...
  • ccextractor -autoprogram -out=srt -latin1 4e56e88ba4...
  • ccextractor -autoprogram -out=ttxt -latin1 c0d2fba8c0...
  • ccextractor -autoprogram -out=ttxt -latin1 27d7a43dd6...
  • ccextractor -autoprogram -out=ttxt -latin1 efbe129086...
  • ccextractor -autoprogram -out=ttxt -latin1 e2e2b501e0...
  • ccextractor -autoprogram -out=ttxt -latin1 -datets dcada745de...
  • ccextractor -autoprogram -out=srt -latin1 -teletext -tpage 398 3b276ad8bf...
  • ccextractor -autoprogram -out=smptett -latin1 -ucla e274a73653...
  • ccextractor -autoprogram -out=ttxt -latin1 -ucla -xds b22260d065...
  • ccextractor -out=smptett c83f765c66...
  • ccextractor -out=spupng c83f765c66...
  • ccextractor --capfile /repository/Dictionary/MattS_dictionary.txt c83f765c66...
  • ccextractor --startcreditsnotbefore 1 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --startcreditsnotafter 2 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --startcreditsforatleast 1 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --startcreditsforatmost 2 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --endcreditstext "CCextractor Ends crdit Testing" addf5e2fc9...
  • ccextractor --endcreditsforatleast 3 --endcreditstext "CCextractor Ends crdit Testing" addf5e2fc9...
  • ccextractor --endcreditsforatmost 2 --endcreditstext "CCextractor Ends crdit Testing" addf5e2fc9...
  • ccextractor -tpage 801 4e56e88ba4...
  • ccextractor -tverbose 4e56e88ba4...
  • ccextractor -teletext 4e56e88ba4...
  • ccextractor -autoprogram -out=srt -bom -latin1 8849331dda...
  • ccextractor -stdout -quiet -nofc -nodvbcolor 79a51f3500...
  • ccextractor -stdout -quiet -nofc -nodvbcolor 767b546f96...

Check the result page for more info.

if (FT_Load_Char(face, current_char_code, FT_LOAD_RENDER)) continue; // ignore errors
// Parse tags like <i> and <b>
if (end) {
if (current_char_code == '>') {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that if we meet tags like <b><i>test</i> test2</b> (if that is valid), it would output "test test2" instead of "test test2"

continue;
}

slot = current_face->glyph;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better and easier to decide which typeface to use here based on the value of italics and bold, instead of changing current_face in ifs.

@harrynull
Copy link
Contributor

So I took a quick look at 608 decoder and it seems that <u> (underline) might also be a valid tag whereas <b> does not seem to be used. Also, it might be possible that the captions contain color information so I'd suggest you to find a sample containing colored captions and see if you could support that too.

Copy link
Contributor Author

@jshin313 jshin313 left a comment

Choose a reason for hiding this comment

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

Should I just implement something for the underline tag and font color then and just get rid of the bold? Are there any other tags that I should be aware of?

@harrynull
Copy link
Contributor

Should I just implement something for the underline tag and font color then and just get rid of the bold? Are there any other tags that I should be aware of?

Please check the decoders, but as far as I am aware that is all.

@jshin313
Copy link
Contributor Author

jshin313 commented Dec 16, 2019

I added support for color.

I couldn't find a video sample that wanted different font colors in the captions so I just put the following in the spupng_export_string2png function:

str = "<font color=\"#66CDAA\"> This should be aquamarine </font> Regular <i> Italics font</i> <font color=\"#00ff00\">This should be green.</font>"; // Test string

I get the follow output from testing the program with the above string.
sub0001

@jshin313 jshin313 force-pushed the handle_tags branch 2 times, most recently from 89e96ae to 29c0cb8 Compare December 16, 2019 19:57
Copy link
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 PR changes a lot of things (formatting) that are unrelated to the functionality it claims to add, making it difficult to review.

Please don't change someone else's formatting (when we do this, it's a PR that does that just and nothing else).

Instead, send a smaller PR that does exactly what it says it does and nothing else.

@jshin313
Copy link
Contributor Author

jshin313 commented Dec 22, 2019

Are you referring to the changes in the functions' parameters and addition changes to the functions' code when you say formatting or do you mean like spaces and tabs. Can you clarify what you mean by formatting. Thanks for reviewing my code.

@jshin313
Copy link
Contributor Author

Also I still have to add support for underlining.

@jshin313 jshin313 changed the title [FIX] Added support for parsing <i> and <b> tags [FIX] Added support for font colors and italics Dec 23, 2019
@cfsmp3 cfsmp3 merged commit 1e32bee into CCExtractor:master Dec 23, 2019
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.

4 participants