[FFmpeg-devel] [PATCH]ffplay 2/2: Fix subtitle rendering for special cases

[Björn Axelsson]

On Tue, 3 Feb 2009, Michael Niedermayer wrote:

> On Tue, Feb 03, 2009 at 12:21:40AM +0100, Michael Niedermayer wrote:
> > On Mon, Feb 02, 2009 at 11:14:53PM +0100, Bj?rn Axelsson wrote:
> > > >On Sun, Feb 01, 2009 at 11:19:37PM +0100, Bj?rn Axelsson wrote:
> > > >> Second subtitle-related patch.
> > > >>
> > > >> The ffplay.c subtitle renderer (blend_subrect) fails to handle some of
> > > >> the
> > > >> special cases with odd widths, odd heights and/or odd positions. For
> > > >> example it could insert an empty line, wraparound the subrect one pixel
> > > >> vertically, misalign the UV components and generally make a mess of
> > > >> things.
> > > >>
> > > >> This fix could have been smaller, since I didn't care to track down the
> > > >> original error in the offset calculations. Instead I replaced the
> > > >> calculations with an arguably simpler and seemingly more correct
> > > >> implementation.
> > > >
> > > >great, iam in favor of someone rewriting blend_subrect()
> > > >first see "C?dric Schieli [FFmpeg-devel] [RFC] Alpha support" there is
> > > >some alpha blend code in there, with which the subtitle case should be
> > > >merged evetually
> > > >
> > > >second the code should be < 1/2 as big as now
> > > >
> > > >your choice is between either
> > > >A. writing a clean and minimal patch to fix the bug (and supply a clear
> > > >   description of the bug
> > > >B. provide a new (not diffed) implementation that is smaller, cleaner and
> > > >   better (1/2 size and attached benchmarks are required)
> > >
> > > or
> > >
> > > C. Do nothing, since I do not want to spend my time debugging complex code
> > >    that I have already replaced with less buggy and slightly less complex
> > >    code and you do not want to review a non-minimal fix.
> >
> > of course, if you dont have the time or will to provide a clean patch
> > then you dont provide one, case closed.
>
> Besides it would be nice if you could at least open a bugreport on roundup
> so others who have more will&time dont have to redo any debuging work.

Actually, I gave up some sleep and tracked down some of the bugs
instead...

blend_subrect_1.diff:
Fix blend_subrect for subrects positioned on odd rows.

When a subrect starts on an odd row blend_subrect begins with rendering a
single row separately. But then it increases the luma and bitmap pointers
as if it has rendered the normal two rows. The result is a gap after the
first row and random noise at the bottom of the rendered subrect.
This can be seen in basically all subtitles in the sample[1].

blend_subrect_2.diff:
Fix blend_subrect for even-width subrects positioned on odd columns.

When a subrect starts on an odd column, the first column is rendered
separately and the chroma pointers are increased by one as if two columns
were rendered. If the subrect had an even width to start width, the rest
of the subrect has now an odd width and the final column is rendered
separately, also increasing the chroma pointers as if two columns were
rendered. This results in the chroma part of the subtitle getting offset
one additional sample to the right for each row.
The effect can be seen in the first, the sixth and the seventh subtitles
of the sample[1].

(I'll promise to send a reindent patch if this is accepted)

blend_subrect_3.diff:
Fix blend_subrect for some subrects positioned on odd rows.

This bug is somewhat similar to the one fixed by patch 2. Luma and bitmap
pointers weren't increased if (y%2==1 && ((x%2==0 && w%2==1) || (x%2==1 && w%2==0))).
The symptom is that the last column of the subtitle is wrapped around and
rendered as the first column. The effect is visible on subtitles 4, 5 and
possibly 7 in the sample[1].


[1] http://samples.mplayerhq.hu/MPEG-VOB/ClosedCaptions/Starship_Troopers.vob

-- 
Bj?rn Axelsson
-------------- next part --------------
Index: ffplay.c
===================================================================
--- ffplay.c.orig	2009-02-05 01:15:53.000000000 +0100
+++ ffplay.c	2009-02-05 21:16:08.000000000 +0100
@@ -495,6 +495,8 @@
             lum[0] = ALPHA_BLEND(a, lum[0], y, 0);
             cb[0] = ALPHA_BLEND(a >> 2, cb[0], u, 0);
             cr[0] = ALPHA_BLEND(a >> 2, cr[0], v, 0);
+            p++;
+            lum++;
         }
         p += wrap3 - dstw * BPP;
         lum += wrap - dstw - dstx;
-------------- next part --------------
Index: ffplay.c
===================================================================
--- ffplay.c.orig	2009-02-05 01:10:11.000000000 +0100
+++ ffplay.c	2009-02-05 21:16:23.000000000 +0100
@@ -521,8 +521,10 @@
             lum[0] = ALPHA_BLEND(a, lum[0], y, 0);
             cb[0] = ALPHA_BLEND(a1 >> 2, cb[0], u1, 1);
             cr[0] = ALPHA_BLEND(a1 >> 2, cr[0], v1, 1);
+            if(dstw & 1) {
             cb++;
             cr++;
+            }
             p += -wrap3 + BPP;
             lum += -wrap + 1;
         }
-------------- next part --------------
Index: ffplay.c
===================================================================
--- ffplay.c.orig	2009-02-05 01:09:26.000000000 +0100
+++ ffplay.c	2009-02-05 21:16:28.000000000 +0100
@@ -496,8 +496,8 @@
             cb[0] = ALPHA_BLEND(a >> 2, cb[0], u, 0);
             cr[0] = ALPHA_BLEND(a >> 2, cr[0], v, 0);
         }
-        p += wrap3 + (wrap3 - dstw * BPP);
-        lum += wrap + (wrap - dstw - dstx);
+        p += wrap3 - dstw * BPP;
+        lum += wrap - dstw - dstx;
         cb += dst->linesize[1] - width2 - skip2;
         cr += dst->linesize[2] - width2 - skip2;
     }