ccextractor icon indicating copy to clipboard operation
ccextractor copied to clipboard

Published 20 hours ago • verified publisher icon CCExtractor

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

Open ArchitBhonsle opened this issue 1 year ago • 23 comments

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

  • [x] I have read and understood the contributors guide.
  • [x] I have checked that another pull request for this purpose does not exist.
  • [x] I have considered, and confirmed that this submission will be valuable to others.
  • [x] I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • [x] 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.
  • [x] 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}

ArchitBhonsle avatar Mar 16 '23 11:03 ArchitBhonsle

@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 avatar Mar 18 '23 07:03 ArchitBhonsle

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?

ArchitBhonsle avatar Mar 18 '23 15:03 ArchitBhonsle

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.

cfsmp3 avatar Mar 18 '23 16:03 cfsmp3

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.

ccextractor-bot avatar Mar 18 '23 16:03 ccextractor-bot

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

ArchitBhonsle avatar Mar 18 '23 17:03 ArchitBhonsle

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?)

canihavesomecoffee avatar Mar 18 '23 20:03 canihavesomecoffee

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 :-)

cfsmp3 avatar Mar 20 '23 01:03 cfsmp3

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

ArchitBhonsle avatar Mar 20 '23 09:03 ArchitBhonsle

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

ArchitBhonsle avatar Mar 20 '23 09:03 ArchitBhonsle

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.

ccextractor-bot avatar Mar 20 '23 10:03 ccextractor-bot

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

ArchitBhonsle avatar Mar 20 '23 15:03 ArchitBhonsle

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.

ccextractor-bot avatar Mar 20 '23 17:03 ccextractor-bot

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!)

ArchitBhonsle avatar Mar 20 '23 17:03 ArchitBhonsle

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 avatar Mar 20 '23 17:03 PunitLodha

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

PunitLodha avatar Mar 20 '23 17:03 PunitLodha

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.

ccextractor-bot avatar Apr 10 '23 10:04 ccextractor-bot

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 avatar Apr 11 '23 09:04 ArchitBhonsle

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 avatar Apr 11 '23 09:04 ArchitBhonsle

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 avatar Apr 11 '23 09:04 ArchitBhonsle

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.

ArchitBhonsle avatar Apr 11 '23 15:04 ArchitBhonsle

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.

ccextractor-bot avatar Apr 11 '23 16:04 ccextractor-bot

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 avatar Apr 12 '23 22:04 PunitLodha

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 avatar Apr 12 '23 22:04 PunitLodha