GNOME Bugzilla – Bug 429319
[alphacolor] distorts png images without alpha channel
Last modified: 2007-04-25 15:57:34 UTC
Please describe the problem: If the png has an alpha channel it works correctly, otherwise, it distorts the image. Steps to reproduce: 1. generate a pipeline with a pngdec and alfacolor element 2. feed a png image with no alpha channel to the filesrc 3. display using a videosink Actual results: the image is distorted instead of being normal Expected results: a normally decoded png image Does this happen every time? yes Other information:
Created attachment 86500 [details] [review] patch to fix 24bit rgb distortion Adds RGB and BGR caps for the sinkpad. Adds right computation for these formats.
*** Bug 429290 has been marked as a duplicate of this bug. ***
Created attachment 86558 [details] [review] right patch to fix 24bit rgb distortion Adds 24bit RGB and BGR caps to alphacolor Adds input stream color depth checking that lacked in the CVS or 0.10.5 version of plugins-good
Created attachment 86715 [details] [review] patch for this bug as above
Created attachment 86826 [details] [review] new patch for CVS alphacolor
The real problem here is that pngdec outputs RGB24 data without an alpha channel, a format that is incompatible with alphacolor's sink pad template caps. Yet the core doesn't realise this for some reason and instead of erroring out, alphacolor's _set_caps() function is called and doesn't notice the mistake either. I believe the core issue is bug #421543 (although I haven't actually tested the patch there). I've commited some fixes to alphacolor that make sure the input caps are in fact RGBA caps now, so that it errors out properly when that is not the case: 2007-04-25 Tim-Philipp Müller <tim at centricular dot net> * gst/alpha/gstalphacolor.c: (gst_alpha_color_base_init), (gst_alpha_color_transform_caps), (gst_alpha_color_set_caps): Double-check that RGB input caps are really RGBA caps (apparently the core doesn't always catch it if those caps aren't a subset of our template caps, also see #421543). Fixes #429319 in a way. Also, don't leak the pad template in the transform_caps function. * tests/check/Makefile.am: * tests/check/elements/.cvsignore: * tests/check/elements/alphacolor.c: (setup_alphacolor), (cleanup_alphacolor), (create_caps_rgb24), (create_caps_rgba32), (create_buffer_rgb24_3x4), (create_buffer_rgba32_3x4), (GST_START_TEST), (alphacolor_suite): Add some basic unit tests for alphacolor. I've also committed a fix to pngdec that makes it post any error messages before the eos, so that gst-launch actually shows the error and doesn't think everything went just fine. About your patch (thanks for that, btw): a) It raises the question whether wewould want RGB24 => AYUV functionality in alphacolor int the first place. I think the main advantage of alphacolor is that it does colorspace conversion in-place, ie. it doesn't allocate a new buffer for each conversion. If you convert RGB24 => AYUV, you need to allocate a new buffer; this functionality could of course be added to alphacolor, but I'm not really sure if it's required - plugging an ffmpegcolorspace is just as good, isn't it? It doesn't buy you anything besides convenience. b) Code-wise: the current alphacolor code uses the GstBaseTransform::in_place() vfunc - if you want to accept RGB24 as input format you need to implement the right GstBaseTransform methods in the right way so that in_place transform and 'normal' transform is chosen as appropriate etc. What you're doing is a bit hackish and it leaks too if I'm not mistaken. You'll need to implement a ::get_unit_size() vfunc and a normal ::transform() vfunc IIRC. If you do want RGB24=>AYUV support in alphacolor, then please file a new enhancement bug (or clone this bug). For your pipeline you probably just want something like: filesrc ! pngdec ! ffmpegcolorspace ! alphacolor ! .... The ffmpegcolorspace will operate in pass-through mode if pngdec outputs buffers in RGBA and no conversion is needed.