[FFmpeg-devel] [PATCH] ffv1dec: Ensure that pixel format constraints are respected

[Michael Niedermayer]

On Thu, Jul 19, 2018 at 05:06:09PM +0200, Vittorio Giovara wrote:
> On Wed, Jul 18, 2018 at 10:37 PM, Michael Niedermayer <
> michael at niedermayer.cc> wrote:
> 
> > On Wed, Jul 18, 2018 at 11:03:47AM -0400, Derek Buitenhuis wrote:
> > > On Wed, Jul 18, 2018 at 10:01 AM, Vittorio Giovara
> > > <vittorio.giovara at gmail.com> wrote:
> > > >> this does not follow from what you write below. But more so this is
> > not
> > > >> how pixel formats were implemented in FFmpeg.
> > > >> Where does this idea come from ?
> > > >
> > > > I found the following description of this pixel format pretty accurate:
> > > > https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-yuv410.html
> > > >
> > > > I am not sure how non-mod4 sizes work, but probably codecs within
> > ffmpeg
> > > > take into account more alignment than necessary and make things work
> > >
> > > To expand on this (and other replies): The behavior in FFmpeg is very
> > unexpected
> > > for an API user who may wish to actually use the returned yuv410p data
> > with an
> > > application or other library that is *not* from FFmpeg, such as a
> > renderer, or
> > > external encoder lib, resizer, etc. Everything else on the planet
> > > assumes that if
> > > you have a buffer of a specific chroma subsampling type, you actually
> > > have enough
> > > data for it to be valid, with width/height that make it so. It's very
> > > surprising when
> > > you get back a set of buffers/width/height that don't make sense for a
> > > given pixel
> > > format, and is little to no documentation about why/how.
> >
> > I think i see what you mean.
> > But iam not sure i understand how the proposed changes would be the ideal
> > solution.
> >
> 
> The change would guarantee that the received buffer respects the pixel
> format constraints,

I think there is possibly some disagreement on what the 
"pixel format constraints" are or should be, judging from the messages here


> and informs the users that there are some lines and
> columns that ought to be cropped away in order to display the desired (or
> valid) output.
> 
> 
> > For example:
> > lets assume we have a 3x3 image of 420 or 410 (be that from a jpeg or
> > whatever)
> >
> > If we want to use this with software that that does support odd resolutions
> > then it should just work. Theres no 4th column or row in the logic image
> > that
> > could be used.
> >
> > OTOH if we want to use this with softwate that does not support odd
> > resolutions
> > then its not going to work with a 3x3 (as odd) or 2x2 (looses a column and
> > row)
> > or a 4x4 (which has a column and a row that is uninitialized or black)
> >
> 
> The problem with this reasoning is that things "happen" to work only
> because ffmpeg decoders always over-allocate the output buffers and make
> sure that there is addressable data. So as long as you stay "within" the
> ffmpeg APIs, everything is perfect, however when you use such data in third
> party libraries you risk running in these issues.

I read this paragraph twice but i dont think this really says a word about
what "the problem" is. it just asserts that there is one


> 
> 
> > what i mean is that the API by which one exports the width and height
> > doesnt
> > really affect this. To get from a 3x3 to something that actually works with
> > external code which only supports even resolutions, something somewhere has
> > to make it even and either scale, crop or fill in.
> >
> 
> Well, IMO the API is low level enough that it would make sense to always
> return the coded sizes and let the user crop to the desired resolution.

Thats not how most decoders work currently in FFmpeg. Changing the behavior of
2 decoders out of many for one pixel format out of many is certainly 
not a good idea no matter on which side of the argument one is on.

More so this is a change in behavior which should be discussed on ffmpeg-devel
and not just "done" by a patch to some decoders.
I mean this is the wrong thread to discuss if we want to make such a change.
A thread to discuss it should make clear in its title what it is about and not
be specific to a single decoder


> Yes several filters/encoders and user apps would need some tweaks, but
> since this is the n-th case in which cropping produces broken results maybe
> it's worth it.
> 
> 
> > More specifically, saying that this 3x3 is a 4x4 image with crop is not
> > really
> > true as theres not neccesarily any data in the last column and row. And a
> > filter or encoder using that anyway could produce bad output
> >
> 
> There is always data in the last column and row, or it would have been
> impossible to encode the buffer.

Taken litterally, what you say would mean that i could not write (on paper
or in a file)

5 8 9
1 2 3
9 9 9

1 2
2 3

1 1
2 2

as 5 8 9 1 2 3 9 9 9 1 2 2 3 1 1 2 2 
but would have to store non existing samples to pad to 4x4
That doesnt make any sense.
So i guess you meant something else, but its not really clear to
me what you meant


> Whether it contains visible data or garbage is another story.
> 
> > I don't think "logic guarantees the buffer is mod4/aligned" is a
> > > reasonable thing
> > > to tell an API user in lieu of documentation for unexpected behavior.
> >
> > just posted a small patch to document this
> >
> 
> I added some padding/cropping code to my program, so that's enough for now,
> but i think a more thorough solution should be found.
> -- 
> Vittorio
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180720/fb7b5d95/attachment.sig>