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

ArchitBhonsle · 2023-03-16T11:24:39Z

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.

{pull request content here}

ArchitBhonsle · 2023-03-18T07:45:26Z

@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 · 2023-03-18T15:40:07Z

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 · 2023-03-18T16:06:32Z

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.

ArchitBhonsle · 2023-03-18T17:01:11Z

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

canihavesomecoffee · 2023-03-18T20:46:20Z

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 · 2023-03-20T01:42:05Z

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 · 2023-03-20T09:15:29Z

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

ArchitBhonsle · 2023-03-20T09:17:53Z

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

PunitLodha

You'll need to define all the 708 structs/enums in rust itself as the next step, before moving forward with any other changes.

src/lib_ccx/ccx_decoders_structs.h

src/lib_ccx/general_loop.c

PunitLodha · 2023-03-20T12:07:20Z

src/rust/src/decoder/mod.rs

-    pub decoders: Vec<&'a mut dtvcc_service_decoder>,
-    pub packet: Vec<u8>,
+    pub report: *mut ccx_decoder_dtvcc_report,
+    pub decoders: [Option<Box<dtvcc_service_decoder>>; CCX_DTVCC_MAX_SERVICES],


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

Should that be a separate PR since it seems to be independent of this one?

Should be in this PR as I think it will allow you to remove the additional Boxing required here, and also will make the code more idiomatic rust as we can stop relying on the C bindings

ArchitBhonsle · 2023-03-20T15:54:17Z

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

ArchitBhonsle · 2023-03-20T17:34:06Z

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 · 2023-03-20T17:50:30Z

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 · 2023-03-20T17:53:15Z

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

ArchitBhonsle · 2023-04-11T09:24:38Z

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 · 2023-04-11T09:28:51Z

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 · 2023-04-11T09:46:13Z

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 · 2023-04-11T15:33:22Z

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.

PunitLodha · 2023-04-12T22:08:50Z

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 · 2023-04-12T22:11:05Z

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

ArchitBhonsle mentioned this pull request Mar 16, 2023

Initialize data structures correctly for the rust CEA-708 decoder #1499

Open

5 tasks

ArchitBhonsle force-pushed the rust-persistent-dtvcc branch from 2ae1735 to 88c238a Compare March 17, 2023 16:09

This comment was marked as off-topic.

Sign in to view

This comment was marked as outdated.

Sign in to view

PunitLodha requested changes Mar 20, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

ArchitBhonsle added 9 commits April 10, 2023 14:58

Rewriting Dtvcc::new() to instantiate

395a873

Moving raw pointers to Options and Vecs to arrays

5e72dff

Questionable implementation of ccxr_dtvcc_free

9ff02b9

Helpful comments

7a0a2fd

Modifying lib_cc_decode (C) to use the new Dtvcc (Rust)

20271f6

Silencing clippy

4b08981

Setting the encoder from C

44141b1

Storing pointers over references and Boxing the decoders

125e740

Switching over the encoder assignment based on DISABLE RUST

e42fe35

ArchitBhonsle force-pushed the rust-persistent-dtvcc branch from ad65396 to c9d9e23 Compare April 10, 2023 09:28

This comment was marked as outdated.

Sign in to view

Exporting ccxr_process_data from Rust and using in mp4.c

22f5684

ArchitBhonsle force-pushed the rust-persistent-dtvcc branch from c9d9e23 to 22f5684 Compare April 11, 2023 15:30

This comment was marked as outdated.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ArchitBhonsle commented Mar 16, 2023 •

edited

ArchitBhonsle commented Mar 18, 2023

ArchitBhonsle commented Mar 18, 2023

cfsmp3 commented Mar 18, 2023

This comment was marked as off-topic.

ArchitBhonsle commented Mar 18, 2023

canihavesomecoffee commented Mar 18, 2023

cfsmp3 commented Mar 20, 2023

ArchitBhonsle commented Mar 20, 2023

ArchitBhonsle commented Mar 20, 2023

This comment was marked as outdated.

PunitLodha left a comment

PunitLodha Mar 20, 2023

ArchitBhonsle Mar 20, 2023

PunitLodha Mar 20, 2023

ArchitBhonsle commented Mar 20, 2023 •

edited

This comment was marked as outdated.

ArchitBhonsle commented Mar 20, 2023

PunitLodha commented Mar 20, 2023

PunitLodha commented Mar 20, 2023

This comment was marked as outdated.

ArchitBhonsle commented Apr 11, 2023 •

edited

ArchitBhonsle commented Apr 11, 2023

ArchitBhonsle commented Apr 11, 2023

ArchitBhonsle commented Apr 11, 2023

This comment was marked as outdated.

PunitLodha commented Apr 12, 2023

PunitLodha commented Apr 12, 2023

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

Are you sure you want to change the base?

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

Conversation

ArchitBhonsle commented Mar 16, 2023 • edited

ArchitBhonsle commented Mar 18, 2023

ArchitBhonsle commented Mar 18, 2023

cfsmp3 commented Mar 18, 2023

This comment was marked as off-topic.

ArchitBhonsle commented Mar 18, 2023

canihavesomecoffee commented Mar 18, 2023

cfsmp3 commented Mar 20, 2023

ArchitBhonsle commented Mar 20, 2023

ArchitBhonsle commented Mar 20, 2023

This comment was marked as outdated.

PunitLodha left a comment

Choose a reason for hiding this comment

PunitLodha Mar 20, 2023

Choose a reason for hiding this comment

ArchitBhonsle Mar 20, 2023

Choose a reason for hiding this comment

PunitLodha Mar 20, 2023

Choose a reason for hiding this comment

ArchitBhonsle commented Mar 20, 2023 • edited

This comment was marked as outdated.

ArchitBhonsle commented Mar 20, 2023

PunitLodha commented Mar 20, 2023

PunitLodha commented Mar 20, 2023

This comment was marked as outdated.

ArchitBhonsle commented Apr 11, 2023 • edited

ArchitBhonsle commented Apr 11, 2023

ArchitBhonsle commented Apr 11, 2023

ArchitBhonsle commented Apr 11, 2023

This comment was marked as outdated.

PunitLodha commented Apr 12, 2023

PunitLodha commented Apr 12, 2023

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

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

ArchitBhonsle commented Mar 16, 2023 •

edited

ArchitBhonsle commented Mar 20, 2023 •

edited

ArchitBhonsle commented Apr 11, 2023 •

edited