[FFmpeg-devel] [FFmpeg-cvslog] avutil/imgutils: remove special case for aligning the palette

[Derek Buitenhuis]

On 2/20/2016 9:24 PM, Michael Niedermayer wrote:
>> This is a silent API break. You changed behavior of a function in such a way
>> that functioning code no longer works.
> 
> yes, i posted a patch that would have maintained API more but people
> did not like it

Yes, I must have missed it. Perhaps it got drowned in a sea of Mats self-replies.
Sorry about that.

> peer review said:
> "> I'd totally expect each line _and_ the start of the palette to be
>  > aligned to the requested slignment.
> 
>  It's what I would expect as well."

I would argue, also, that I would not expect a "GRAY8" plane to:

1. Have a palette.
2. Require alignment.

Grey is grey. Not colored with a palette.

But I digress, that matter is beyond the scope here, I suppose.

> so i did that and i added the check above to catch the case where
> this results in unaligned AVFrame.
> dependig on how the AVFrame is used that can be a problem or can also
> be no problem.

[...]

> should i remove this check ? (this would be undefined behavior if
> someone accesses the palette with int*, i belive there is some code in
> our codebase which does this ...)

Aside: An alignment of 4 is not going to help if someone accesses it as int*
on a platform with 64 bit ints.

> should i replace it by a warning ?
> 
> should i revert the whole patchset (that will result in generated raw
> rgb files to be invalid for nut, avi and mov)

[...]

> should i revert this and apply the other patchset that maintains API
> more but that was rejected by people ?

I didn't see this one either; I'll try and go back and look.

> do you suggest something else ?
> also iam very happy to leave this to others, if someone else wants to
> take over, its rather difficult to implement this in a way that makes
> everyone happy.

I'm truly not sure. In my view, the technically correct thing to do
is to introduce a new API function (according to Semantic Versioning).
This introduces yet another some_function2, which is more API churn...

I am not sure how many people outside of FFMS2 this has/will bite, or
what is the "least bad" fix. I only noticed since FFMS2 apparently ceased
to function entirely after that commit; I doubt anyone else hardcodes
GRAY8.

I am hoping the two people named in the commit (Stefano and wm4) can
offer some insight.

- Derek