search

CCExtractor / ccextractor

CCExtractor - Official version maintained by the core team
https://www.ccextractor.org
GNU General Public License v2.0
721 stars 426 forks source link

[WIP] [FIX] #1499 Persistent `Dtvcc` struct for CEA-708 decoding in Rust #1501

Closed ArchitBhonsle closed 3 months ago

ArchitBhonsle commented 1 year ago

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

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

{pull request content here}

ArchitBhonsle commented 1 year ago

@PunitLodha This builds but segfaults when ran on this sample. I'll try to diagnose this but could you go over the changes when you get the time?

ArchitBhonsle commented 1 year ago

Using ugly debug statements I have pinned down the segfault to here. It's exactly where the rust function is called. Not inside it as I put a debug statement on the Rust side but that did not get triggered. Any ideas why?

cfsmp3 commented 1 year ago

Using ugly debug statements I have pinned down the segfault to here. It's exactly where the rust function is called. Not inside it as I put a debug statement on the Rust side but that did not get triggered. Any ideas why?

I'd run it with valgrind, it will give you a good idea.

ccextractor-bot commented 1 year ago

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
CEA-708 0/14
DVB 0/7
DVD 0/3
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Options 0/87
Teletext 0/21
WTV 0/13
XDS 0/34

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.

ArchitBhonsle commented 1 year ago

Is there a way to disable this longer check for a draft PR XD

canihavesomecoffee commented 1 year ago

Is there a way to disable this longer check for a draft PR XD

No, but that'd be a good addition for the SP so that Drafts aren't tested anymore (unless @cfsmp3 sees value in testing draft PR's?)

cfsmp3 commented 1 year ago

Is there a way to disable this longer check for a draft PR XD

No, but that'd be a good addition for the SP so that Drafts aren't tested anymore (unless @cfsmp3 sees value in testing draft PR's?)

Not much :-)

ArchitBhonsle commented 1 year ago

It works?! It does not seem perfect (lacks the "footer" for one) but it does produce a file! video.p1.svc01.smi.txt

ArchitBhonsle commented 1 year ago

I had a nasty bug where the program just crashed when I added a single line Dtvcc::new. Kept thinking it was some sort of undefined behavior because of all the pointer magic but it was just because I allocated too much on the stack in my quest to "optimize".

ccextractor-bot commented 1 year ago

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
CEA-708 0/14
DVB 0/7
DVD 0/3
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Options 0/87
Teletext 0/21
WTV 0/13
XDS 0/34

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.

ArchitBhonsle commented 1 year ago

This commit conditionally switches between the two. Oh yes, I have not fixed the encoder assignment for mp4. Should I do that too?

ccextractor-bot commented 1 year ago

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
CEA-708 0/14
DVB 0/7
DVD 0/3
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Options 0/87
Teletext 0/21
WTV 0/13
XDS 0/34

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.

ArchitBhonsle commented 1 year ago

Wait a minute... does this fix #1500 ? I ran this on the link mentioned on the video mentioned here and it works. Here's the SMI output: video.p0.svc01.smi.txt (It's in lowercase!)

PunitLodha commented 1 year ago

This commit conditionally switches between the two. Oh yes, I have not fixed the encoder assignment for mp4. Should I do that too?

Yes

PunitLodha commented 1 year ago

Wait a minute... does this fix #1500 ? I ran this on the link mentioned on the video mentioned here and it works. Here's the SMI output: video.p0.svc01.smi.txt (It's in lowercase!)

That sounds great. Ig all 708 problems stem from the same issue

ccextractor-bot commented 1 year ago

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
CEA-708 0/14
DVB 0/7
DVD 0/3
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Options 0/87
Teletext 0/21
WTV 0/13
XDS 0/34

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.

ArchitBhonsle commented 1 year ago

I just noticed that there is a difference between the 608 and 708 output, is this expected?

8e8229b88bc6b3cecabe6d90e6243922fc8a0e947062a7abedec54055e21c2bf.p1.svc01.smi.txt

8e8229b88bc6b3cecabe6d90e6243922fc8a0e947062a7abedec54055e21c2bf.smi.txt

Footer: When I made this comment I was specifically referring to the difference in timings but there's a slight difference in timings too. And 708 doesn't have a footer.

ArchitBhonsle commented 1 year ago

Oh and I forgot to inform you. @PunitLodha I've removed the formatting changes from src/lib_ccx/ccx_decoders_structs.h now. So should be easier to go through.

ArchitBhonsle commented 1 year ago

There is no longer a need to use bindings for any 708 related structs/enums, we can define them now in rust itself. We still need to use bindings for encoder, timing and report. But all others should be defined in rust. This is going to be somewhat bigger and comprehensive change

@PunitLodha I mean the only other on dtvcc_service_decoder right? Which in turn contains dtvcc_window and dtvcc_tv_screen. All of them have a significant number of functions associated with them.

I'm sure porting them over to Rust would be better than calling them over FFI. I still think a PR started to specifically fix the persistence of the Dtvcc struct is the wrong place to do it as it blocks merging the PR which does seem to fix quite a bugs.

ArchitBhonsle commented 1 year ago

Dammit another formatting change slipped in. Basically I made some changes to src/lib_ccx/mp4.c and src/rust/src/lib.rs in an attempt to fix the mp4 slow but I'm really not sure if that's correct.

ccextractor-bot commented 1 year ago

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
CEA-708 0/14
DVB 0/7
DVD 0/3
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Options 0/87
Teletext 0/21
WTV 0/13
XDS 0/34

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.

PunitLodha commented 1 year ago

I'm sure porting them over to Rust would be better than calling them over FFI. I still think a PR started to specifically fix the persistence of the Dtvcc struct is the wrong place to do it as it blocks merging the PR which does seem to fix quite a bugs.

Currently there's a lot of unnecessary unsafe code, which can be avoided if we move away from the bindings

PunitLodha commented 1 year ago

Dammit another formatting change slipped in. Basically I made some changes to src/lib_ccx/mp4.c and src/rust/src/lib.rs in an attempt to fix the mp4 slow but I'm really not sure if that's correct.

Seems correct, just need to revert the formatting changes

PunitLodha commented 3 months ago

Closing since #1618 implements this