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

dvd: add widescreen vobsubs when letterbox is permitted #355

Merged
merged 2 commits into from Feb 21, 2017

Conversation

2 participants
@jstebbins
Copy link
Contributor

commented Oct 13, 2016

We were only adding subtitle tracks specifically designed for letterbox
when letterbox is permitted. We should also add the subtitle tracks
that are specifically designed for wide screen.

libhb/dvd.c Outdated
switch (style)
{
case HB_VOBSUB_STYLE_4_3:
strcat( subtitle->lang, " (4:3)" );

This comment has been minimized.

Copy link
@bradleysepos

bradleysepos Oct 13, 2016

Contributor

Nit: brackets here instead of parens?

This comment has been minimized.

Copy link
@jstebbins

jstebbins Oct 13, 2016

Author Contributor

Do you mean in the text? e.g. [4:3]? Why would that be "better"? Parens match the other descriptive items we append to the description.

This comment has been minimized.

Copy link
@bradleysepos

bradleysepos Oct 13, 2016

Contributor

In my experience, brackets are commonly used for technical additions. Similar to in jurnalizm [sic]. No biggie, just avoids multiple sets of parens.

This comment has been minimized.

Copy link
@jstebbins

jstebbins Oct 13, 2016

Author Contributor

The description shown in the CLI and GUI does have a lot of parens. Adding different punctuation is going to look a bit strange.
e.g.
CLI: English (Closed Caption) (Wide Screen) (iso639-2: eng) (Bitmap)(VOBSUB)
GUI: English (Closed Caption) (Wide Screen) (VOBSUB)

I was just being a good lemming.

We could probably eliminate a lot of what the CLI is showing. I'm pretty sure it was there so the WinGui could parse out everything it needed to know when it was a CLI wrapper.

How about reformatting the whole thing to something like this?
English, Closed Caption [Wide Screen, VOBSUB]

The problem with doing this is it makes the patch much more extensive. Everything after the "(Wide Screen)" part is added by the frontend. So we would have to go around and change all the frontends as well. I would suggest just having libhb build the full description and remove this extra code from the frontends.

But now this is heading in directions this patch really wasn't intended to go. So my suggestion is that reformatting should be a separate patch as well.

This comment has been minimized.

Copy link
@bradleysepos

bradleysepos Oct 13, 2016

Contributor

Agreed. Sounds like a plan.

Now I want to play Lemmings. DOS, of course!

strcat( subtitle->lang, " (Closed Caption)" );
break;
case 6:
strcat( subtitle->lang, " (Closed Caption with bigger size character)" );

This comment has been minimized.

Copy link
@bradleysepos

bradleysepos Oct 13, 2016

Contributor

Is "with bigger size character" a standard description? If not, I suggest "Closed Caption, Large Type". Same for other instances.

This comment has been minimized.

Copy link
@jstebbins

jstebbins Oct 13, 2016

Author Contributor

That would be for a different patch. This is the wording we currently use. I didn't change it.

This comment has been minimized.

Copy link
@bradleysepos

bradleysepos Oct 13, 2016

Contributor

Okay. Wasn't sure whether it was new.

@bradleysepos

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

Great work, John. Initial read looks good.

@bradleysepos bradleysepos added this to the 1.1.0 milestone Oct 13, 2016

@jstebbins jstebbins self-assigned this Jan 26, 2017

jstebbins added some commits Oct 13, 2016

dvd: add widescreen vobsubs when letterbox is permitted
We were only adding subtitle tracks specifically designed for letterbox
when letterbox is permitted.  We should also add the subtitle tracks
that are specifically designed for wide screen.

@jstebbins jstebbins force-pushed the jstebbins:vobsub branch from 7f83df3 to 07b630b Feb 21, 2017

@jstebbins jstebbins merged commit e2b3a1e into HandBrake:master Feb 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.