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] Added support for encoding into an MCC File. (CCExtractor#733) #1097

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@robdt
Copy link

commented Jun 25, 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 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.

{pull request content here}

@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

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

@canihavesomecoffee canihavesomecoffee requested review from cfsmp3 and saurabhshri Jul 9, 2019

@canihavesomecoffee

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Hi @robdt,

Can you take a look at these errors for the Windows build environment?

"ccextractor\windows\ccextractor.sln" (default target) (1) ->
"ccextractor\windows\ccextractor.vcxproj" (default target) (2) ->
(Link target) ->
  mp4.obj : error LNK2019: unresolved external symbol _mcc_encode_cc_data referenced in function _process_clcp [ccextractor\windows\ccextractor.vcxproj]
  ccx_decoders_common.obj : error LNK2001: unresolved external symbol _mcc_encode_cc_data [ccextractor\windows\ccextractor.vcxproj]
  Debug\ccextractorwin.exe : fatal error LNK1120: 1 unresolved externals [ccextractor\windows\ccextractor.vcxproj]
 
    1188 Warning(s)
    3 Error(s)

Looks like the file should be just added to the .sln 👍

@robdt

This comment has been minimized.

Copy link
Author

commented Jul 11, 2019

Hey @canihavesomecoffee,

Thanks! It looked like it should be added into the Visual C Project file, so I added it there. There weren't any other files in the .sln like there were in the .vcxproj. I am hoping that is correct. I am still looking at why there are issues with the tests. I think it is related to the command line option I added. Please let me know if you see that it didn't fix the problem with Windows. I am still a little handicapped there as I don't have access to a Windows box or Visual Studio to build and test.

@canihavesomecoffee

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Hi @robdt,

Thanks for the changes! We're down to one issue now: the mcc file requires uuid/uuid.h, which is not available on Windows.

As this library is not natively available (afaik) for Windows, we might have to look into a solution as mentioned here: djp952/prebuilt-libuuid#1 (comment)

@cfsmp3 What would you prefer to do?

@cfsmp3

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

The less dependencies the better - if we can copy/paste the code for those functions from a well tested source I'd do that, unless there's a good reason not to.

@robdt

This comment has been minimized.

Copy link
Author

commented Jul 11, 2019

@canihavesomecoffee - That would be easy to pull in. In fact I could make that the code for all platforms if that made more sense than leaving Linux and Mac as they are. Let me know if you want me to proceed down that path.

@cfsmp3

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@robdt yes please do that - let's use that code for all platforms so we can avoid the dependency. I find it helps a lot with portability (and building, in general).

@robdt

This comment has been minimized.

Copy link
Author

commented Jul 11, 2019

Thanks @cfsmp3, will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.