On Sat, 20 Feb 2016 21:38:30 +0000 Derek Buitenhuis <derek.buitenh...@gmail.com> wrote:
> 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. I would agree. GRAY8 "needs" a palette for in-memory AVFrames and such, because PSEUDOPAL guarantees it and libswscale relies on it. But it mustn't require a palette for anything else, which includes fixed frame layouts for interoperation with other code or storage, like the av_image functions are intended for. Can we please deprecate and remove this PSEUDOPAL nonsense? Other than that, I'd argue a function which includes the palette in the layout should probably be separate, because there are many different methods of storing the palette, and appending it to the image data is just one of them. And who says the palette should be aligned? The current code/semantics don't even make sense. > > 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. IMO, this behavior should definitely be removed for PSEUDOPAL. The "real" paletted case doesn't make too much sense either, but might be required for compatibility? _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
