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

Subtitles improvments #2741

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

jstebbins
Copy link
Contributor

Description of Change:

  • Adds to libhb ability to mux to external subtitle files (currently ass and sup). This is not wired to any frontend yet. UIX design work remains.
  • Many ffmpeg patches to improve decoding and encoding of TX3G subs
  • Turnes on TX3G subtitle decoding with ffmpeg. We were still using our own decoder due to missing features in ffmpeg's decoder.
  • Adds a libav subtitle encoder to the subtitles pipeline. This is currently only used for transcoding text subtitles to TX3G for mp4 container output.

Test on:

  • Windows 10+ (via MinGW)
  • macOS 10.13+
  • Ubuntu Linux

@@ -2442,7 +2442,7 @@ ghb_update_summary_info(signal_user_data_t *ud)
}
if (burn)
{
g_string_append_printf(str, ", Burned");
g_string_append_printf(str, ", Burned🔥");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for me, eh? 😹

@bradleysepos
Copy link
Contributor

Impressive.

Kudos for the upstream fixes, and upstreaming HandBrake functionality to FFmpeg. Should benefit many, even those outside HandBrake's reach.

@jstebbins
Copy link
Contributor Author

I haven't submitted patches to ffmpeg yet. I need to run everything through FATE tests and it would be nice to get a bit more than my few test cases thrown at it first.

The patches basically bring ffmpeg up the the level our own decoder/encoder were at, plus one improvement that supports transcoding font tags from ass to tx3g. It's limited to the fonts specified in the ass style header, but it's better than "all fonts are Arial" like we had or "all fonts are Serif" like ffmpeg had.

@bradleysepos
Copy link
Contributor

I haven't submitted patches to ffmpeg yet.

I just assumed you will eventually. 😉

The patches basically bring ffmpeg up the the level our own decoder/encoder were at...

That's what the initial read looked like to me. How long before we stop telling people HandBrake isn't an FFmpeg front end? 😝

@sr55
Copy link
Contributor

sr55 commented Mar 31, 2021

This worth re-visting for 1.4?

@jstebbins
Copy link
Contributor Author

Holy cow, I forgot I even started this. I'm going to have to take some time to go over all this again.

@jstebbins
Copy link
Contributor Author

We should look into the ffmpeg bump before I dig back into this. I may need to adapt patches after the bump.

There is not frontend support yet.  Testing by hard coding values.
So far there are no encoders implemented.  Just a "passthru" stage that
is also used by audio for audio passthru.
Subtitle decoders need the extradata, so make sure it's initialized
before decoders are initialized.
Was getting all underlined text and styles were not getting parsed
properly.
So far it's only used for converting ASS subs to TX3G subs.
@jstebbins
Copy link
Contributor Author

Rebased on master. Did a couple quick tests of SSA to tx3g and back to SSA encoding. But more testing is needed before calling this good (like some valgrind runs).

Might want to add export to external subtitle file capability to at least the CLI since the capability in in libhb, but that can be a follow-on PR.

If anyone has good samples of tx3g that uses more style or font features, please pass them along and/or do some testing.

Thanks!

}
else
{
// TODO: bitmap subtitles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encavsub is currently only used for conversion to tx3g. My plan was to flesh out the bitmap path by writing a pgs encoder for ffmpeg and using it to convert vobsub to pgs. The pgs encoder is written for the most part, but there's work left to do (like getting it through ffmpeg review).

@@ -1091,6 +1103,7 @@ hb_job_t* hb_dict_to_job( hb_handle_t * h, hb_dict_t *dict )
"Forced", unpack_b(&job->select_subtitle_config.force),
"Default", unpack_b(&job->select_subtitle_config.default_track),
"Burn", unpack_b(&subtitle_search_burn),
"ExternalFilename", unpack_s(&subtitle_search_external_filename),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this work? What if there are multiple external subs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for output of external subtitle files, so you can do things like pair an external PGS file with mp4 output (since mp4 doesn't support embedded PGS). Many players will import external subtitles.

The particular example you've highliighted is foreign audio search which will generate one output track.

@sr55
Copy link
Contributor

sr55 commented Jun 5, 2021

Image Subtitles:

  • PGS Subtitles MKV
  • PGS Subtitles MP4 Burn in
  • VobSub Passthru
  • VobSub Burnin

Text Subtitles

Real SRT.zip

@jstebbins
Copy link
Contributor Author

Awesome! thanks for the testing. I'll look into that problem with burnin probably thrusday or friday. My day job has me busy this week 😉

@sr55 sr55 modified the milestones: 1.4.0, 1.5.0 Jul 16, 2021
@bradleysepos bradleysepos removed this from the 1.5.0 milestone Jan 6, 2022
@bradleysepos bradleysepos added this to the 1.6.0 milestone Jan 6, 2022
@bradleysepos
Copy link
Contributor

Re-targeting for 1.6.0.

@sr55 sr55 modified the milestones: 1.6.0, Unscheduled Jul 23, 2022
@3z3qu13l
Copy link

Is there any plan to ship it in the next release ?

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

Successfully merging this pull request may close these issues.

None yet

5 participants