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

[Michael Niedermayer]

On Thu, Feb 05, 2009 at 10:35:35PM +0100, Bj?rn Axelsson wrote:
> 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...

sleeping is a waste of time anyway ;)


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

ok


[...]

> 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].

this change looks wrong


> 
> (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.

ok


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090205/fdbe3441/attachment.pgp>