Improve legibility and display of CEA-608 captions.

[faginbagin <mythtv@…>]

Make sure 608 captions are centered on screen, while still confined to 90% safe area, as long as zoom factor is not too extreme.

Use correct maximum columns (32) and rows (15) for geometry.

Compute a better font.pixelSize by making first guess, then adjusting using QFontMetrics' height and lineSpacing to choose a font.pixelSize that works for all but the most extreme fonts.

Eliminate use of fudge factors that caused a number of problems, such as:

• smaller than necessary font.pixelSize (if a user thinks it's too large, subtitle zoom still works).
• Gaps between caption lines.
• Captions meant for row 14 appearing on row 15.

Make sure top rows are anchored to top of safe area and bottom rows are anchored to bottom of safe area while eliminating unnecessary floating point arithmetic.

More logging at verbosity vbi and log level debug have been added to gain visibility into geometry calculations.

This patch will apply cleanly to master, but it was actually tested on fixes/0.27 after the following files were checked out from master: mythtv/libs/libmythtv/cc608decoder.cpp mythtv/libs/libmythtv/cc708reader.cpp mythtv/libs/libmythtv/cc708window.cpp mythtv/libs/libmythtv/cc708window.h mythtv/libs/libmythtv/mythccextractorplayer.cpp mythtv/libs/libmythtv/subtitlescreen.cpp mythtv/libs/libmythtv/subtitlescreen.h

It was also tested with the bug fix patches from tickets 12051 and 12056. However, all three patches should apply cleanly independent of each other.

[faginbagin <mythtv@…>]

Just discovered a regression I created that affects display of SRT captions. Please use 608-improvements-1.patch instead of 608-improvements.patch.

[Jim Stichnoth]

Thanks for looking so deeply into this!

I have one main comment on the patch.

@@ -690,8 +689,10 @@ bool FormattedTextChunk::PreRender(bool isFirst, bool isLast,
     int leftPadding, rightPadding;
     CalcPadding(isFirst, isLast, leftPadding, rightPadding);
     // Account for extra padding before the first chunk.
-    if (isFirst)
-        x += leftPadding;
+    // No, this will cause everything to be off center to the right
+    // by the value of leftPadding.
+    // if (isFirst)
+    //     x += leftPadding;
     QSize chunk_sz = CalcSize();
     QRect bgrect(x - leftPadding, y,
                  chunk_sz.width() + leftPadding + rightPadding,
@@ -909,6 +910,18 @@ void FormattedTextSubtitle::Draw(void)

I suspect that you aren't using the (black) background in your testing. The background is designed to extend beyond the left and right edges of the text by 0.5 (PAD_WIDTH) character widths on each side. With the leftPadding adjustment removed, the left edge of the text is flush with the background, which doesn't look so good, and also on my system it causes some weird drawing artifacts because (I think) objects are being drawn outside of their bounding rectangles.

It might work to just arrange for CalcPadding?() to always set leftPadding=rightPadding=0 when there is no background, which is when SubtitleFormat::GetBackground?() returns NULL, but simpler to just keep it as is. Especially when we're talking about a difference of a half character against a somewhat arbitrary specification of a 90% "safe region".

For the rest of the patch, I need a little more time to test it out.

[faginbagin <mythtv@…>]

I do indeed use the black background and I'm not seeing the left edge of the text flush with the background. But some time after I submitted the patches in this ticket, I did observe that italics were sometimes clipped on the right with some fonts, particularly Droid Sans Mono and when the last italic char was an E or W, for example. I fixed it by padding the bounding box on the right. I'm afraid I never submitted a patch, but I will now. Perhaps it will fix the problem you are seeing? Might not, because I'm pretty sure that I did confirm the problem existed in 0.27 without any of my patches. I will also attach the osd_subtitle.xml that I've been using. At the time I was working on this, I did test without a custom osd_subtitle.xml, and with one, experimenting with different fonts. IMHO, I find Liberation Mono to be a much better font for CCs than Free Mono or Droid Sans Mono.

[Jim Stichnoth]

I've noticed differences in clipping depending on whether Xv or VDPAU is used. Weird behavior reflecting MythTV area calculation bugs tends to show up with Xv, in my experience.

In any case, I wanted to say that I really like this mechanism for figuring out what the default pixelsize really should be. I'd like to use the calculation uniformly for cc708 and srt captions as well, to maintain the overall consistency between the three caption types. One consequence will be that captions are suddenly larger than before, but this can be quickly and permanently/persistently fixed by updating the subtitle zoom factor.

[faginbagin <mythtv@…>]

I've got an old laptop I use as a frontend that uses the slim profile. I think that uses Xv, right? If so, I can probably test my changes on it, if it would help.

I'm glad you like the mechanism. I had wanted to move it into a base class shared by the 708 and srt captions, too. I just hadn't gotten around to it. I must admit I was kinda frustrated when I hadn't gotten any feedback on my work. But I can probably take this on, unless you would prefer to do it?

[faginbagin <mythtv@…>]

I am attaching a new patch based on offline discussions with stichnot. The highlights:

SubtitleFormat has a new font cache, m_fullFontMap, which caches fonts based on the full set of attributes: family, cc708 attrs, zoom & stretch.

SubtitleFormat::Load implements a default font sizing algorithm shared by all caption types. It chooses a better font.pixelSize by making a first guess, then adjusting using QFontMetrics' height and lineSpacing to choose a font.pixelSize that works for all but the most extreme fonts.

The algorithm takes into account video aspect ratio for 708 and text captions, outline size and shadow offset, font stretch and left & right padding.

If the resulting font metrics lead to a font width that exceeds a max width based on the above, it logs a warning. It does not try to make a third adjustment. I haven't found a test case that triggers that warning.

SubtitleFormat::GetFont depends on Load, to initialize a default font cached in m_fontMap. GetFont then applies more transient changes, such as zoom, color, italics, etc. and caches the result in m_fullFontMap.

To reduce the number of calls to SubtitleFormat::GetFont, FormattedText[Chunk|Line]::Calc methods that were const, aren't anymore and the SubtitleScreen::Calc method now take an optional mythfont argument.

Fixed a memory leak in SubtitleFormat::CreateProviderDefault.

Fixed a problem where left padding was outside the safe area and, when using the Slim display profile, the padding would be clipped. The root cause was due to padding being applied twice in FormattedTextSubtitle::Layout. I addressed it by adding a bool addPadding argument to FormattedTextLine::CalcSize.

Made sure captions are drawn after new ones are added. Don't just depend on bounding rectangle changes. Affects captions using Slim profile that happen to have the same bounding box as the preceding caption. Effect can be seen playing: ​http://ncamftp.wgbh.org/DTV/CEA%20test%20material/Iteration_1/CEAv1.2zero.trp Without this patch, no captions are displayed for slides 3, 4 and 11, because their bounding box is identical to the preceding caption.

[Jim Stichnoth]

Updated to latest master

[Stuart Auchterlonie]

Moving unresolved tickets to next point release

[Stuart Auchterlonie]

Moving remaining open 0.28.1 tickets to 0.28.2

[Stuart Auchterlonie]

Closing any remaining tickets for 0.28, if the issue persists, feel free to reopen and align to v29 or master