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

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

Open
5 tasks
PunitLodha opened this issue Mar 14, 2023 · 13 comments · May be fixed by #1618
Open
5 tasks

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

PunitLodha opened this issue Mar 14, 2023 · 13 comments · May be fixed by #1618
Labels

Comments

@PunitLodha
Copy link
Member

Currently, the entry point for the rust decoder is here where we create a new Dtvcc struct each time. This causes the data to be reset each time ccxr_process_cc_data() is called.

Instead, we want to initialize Dtvcc only once at the start of the program and then use it for all subsequent function calls.

Steps:-

Additionally to fix mp4 code flow:-

  • Call the corresponding rust function here and pass Dtvcc as done in the above steps
@ArchitBhonsle
Copy link
Contributor

ArchitBhonsle commented Mar 15, 2023

I was on the first task. Any idea how the encoder should be instantiated? The current rust code just copies it over from the context it is passed while the C code does not instantiate it at all. Is it null or am I reading it wrong?

@cfsmp3
Copy link
Contributor

cfsmp3 commented Mar 15, 2023

I was on the first task. Any idea how the encoder should be instantiated? The current rust code just copies it over from the context it is passed while the C code does not instantiate it at all. Is it null or am I reading it wrong?

It should start with NULL - please send a PR to correct that in the C code :-) Just a matter of hygiene.
If you look at the actual initilization

https://github.com/CCExtractor/ccextractor/search?q=dtvcc-%3Eencoder

There a couple of places in which it happens. But it should start with NULL when the context is created.

@PunitLodha
Copy link
Member Author

As @cfsmp3 mentioned, it should be initialized to NULL. You should also add a function to set the value for encoder field, which is called whenever the value for dtvcc->encoder is updated in the C code.

On a side note, If you are working on this issue, I recommend you to open a draft PR and keep pushing your changes there, so that I can review each step individually. This issue is quite a bit of work, and reviewing a huge PR would be difficult

@ArchitBhonsle
Copy link
Contributor

If could push my progress but because of how I'm working, the commits themselves would fail to build. Is that okay?

@ArchitBhonsle
Copy link
Contributor

Also how do I approach the compatibility issue. The encoder for example. It would be initialized to NULL but in Rust should I use the Rusty approach of using Option and write getters and setters to work with it in C or use std::ptr::null()? I would prefer the former but it might necessitate more changes on the C side.

@ArchitBhonsle
Copy link
Contributor

ArchitBhonsle commented Mar 16, 2023

And other issue is stuff like dtvcc_tv_screen where in C we just allocate memory for it and assign some of the values while the rest are left as garbage. While instantiating these structs in Rust I have to assign all the values, right? Which is fine for simple values but when there's nested structs (like windows in dtvcc_tv_screen) it can get cumbersome.

Edit: For now I have added dervive_default(true) to the bindgen builder which makes this a lot simpler. But do tell me if you guys have any better ideas.

Edit 2: This conflicts with the manually defined Default implementations for dtvcc_pen_attribs and dtvcc_pen_color . I'll rename these to new for now.

@ArchitBhonsle
Copy link
Contributor

@PunitLodha here's the PR #1501. For now I've just rewritten Dtvcc::new().

@ArchitBhonsle
Copy link
Contributor

ArchitBhonsle commented Mar 18, 2023

It should start with NULL - please send a PR to correct that in the C code :-) Just a matter of hygiene. If you look at the actual initilization

https://github.com/CCExtractor/ccextractor/search?q=dtvcc-%3Eencoder

There a couple of places in which it happens. But it should start with NULL when the context is created.

Hey @cfsmp3. Of the 3 code results in the link you have sent only the first two, general_loop.c and mp4.c are initializations that should affect the Rust decoder right?

@cfsmp3
Copy link
Contributor

cfsmp3 commented Mar 18, 2023

Hey @cfsmp3. Of the 3 code results in the link you have sent only the first two, general_loop.c and mp4.c are initializations that should affect the Rust decoder right?

I haven't tracked it down, but in general any structure should have all its fields initialized with something when it's created :-)

@ArchitBhonsle
Copy link
Contributor

@PunitLodha for the mp4 part, do I just expose another extern function from Rust to call this or somehow reuse this?

@PunitLodha
Copy link
Member Author

You'll need to expose another extern function

@asher-gh
Copy link
Contributor

Seems like #1501 is not active anymore. What's the best way to get ccx_decoder_dtvcc_settings when calling Dtvcc::new in ccxr_process_cc_data here?

@IshanGrover2004
Copy link
Contributor

IshanGrover2004 commented Feb 20, 2024

I have started working on this issue & I have raised a Draft PR #1599 as it is WIP.
Till now I have done the first task which is Changing Dtvcc::new() to create Dtvcc using ccx_decoder_dtvcc_settings

Feedbacks are appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment