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

Vobsub: allow parsing input stream by chunks #41

Closed
wants to merge 2 commits into from

Conversation

fengalin
Copy link

This is a first step toward sdroege/gst-plugin-rs#21

The PR adds a new struct SubtitlesFromChunks which keeps track of the parsing context and allows passing chunks.

Except if I missed something, the API and behaviour for Subtitles is unchanged. I couldn't find a solution to avoid using the Rc<RefCell<>> for SubtitlesContext without breaking the existing API.

Changes were tested using examples in "fixtures" (including for the parse_corpus ignored set).

@fengalin fengalin force-pushed the master branch 3 times, most recently from af4ccd0 to a8b5e00 Compare April 16, 2018 12:59
@fengalin
Copy link
Author

It turns out I will need to process demuxed streams which contain only data and control sequences (no header), so I need extra work on vobsub. I'm thinking about doing it in a separate PR though.

@emk
Copy link
Owner

emk commented Apr 17, 2018

Thank you very much for this PR! I totally want to integrate any changes needed for GStreamer.

I'll have some time tomlook at this the coming weekend. If you have any questions, please don't hesitate to ask. And if you don't hear from me, please feel free to ping me here.

The `needed` length was not available in all cases which made it
useless in some situations in chunk mode.

This commit attempts to guestimate the `needed` length so that
the next chunk is always large enough for the next iteration to
at least give a new `needed` length which will allow progressing
in the stream.
@emk
Copy link
Owner

emk commented Apr 20, 2018

I'm on the road for part of today, but I've pulled your proposed branches locally, and will try to find time to look at them either today or tomorrow.

This looks really great, BTW. I may have a few suggestions, but I like the overall strategy and I'm delighted to have these new features.

(And once the new code is integrated, we'll need to fire up the fuzzer again!)

@fengalin
Copy link
Author

Great to read that!
Please note I've updated #42 around 12:00 UTC today, so you may need to pull the branch again if you cloned it before that time.

@fengalin
Copy link
Author

Hi @emk! Do you think you will get time to review this PR?

@emk
Copy link
Owner

emk commented Apr 28, 2018

Sorry! Just after getting back from vacation, our house-buying schedule got made much shorter by outside events, and I'm currently operating on about 5 hours of sleep a night. I absolutely will review your patches soon, but I'm trying to get a few other distractions out of the way first. I can't give you an exact ETA, but I'm hoping to have some time to really look at these in the next week.

My apologies for the delay. :-( These patches are very welcome—and very interesting—and I really want to dig into them.

@fengalin
Copy link
Author

No problem! I'm not in a rush anyway.

@emk emk closed this May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants