GNOME Bugzilla – Bug 164268
[PATCH][ffmpegcolorspace] Fix BGR <-> RGB handling
Last modified: 2006-05-15 10:01:16 UTC
Here is the situation: with GStreamer caps, the difference between RGB and BGR is the red_mask and blue_mask values. Those are set appropriately. However, img_convert does an RGB<->BGR switch too, which blows up the whole thing. I believe that the appropriate solution here would be to rip out all the BGR stuff from ffmpegcolorspace to get a clean start, and then reimplement BGR handling by lying to img_convert() that it is RGB, and fiddle with the gstreamer caps to get it right. This approach means less code, and more supported colorspaces. If anyone can confirm my suspicion, I'd be thankful. So thankful that I'd volunteer to prepare a patch. (I'm so confused when it comes to colorspaces, that I do not dare to work on this before I get confirmation that I'm not doing something terribly stupid)
Created attachment 36115 [details] [review] Proposed patch Actually, I've just gone and did this, since I figured out it's actually not as hard as I first thought. The patch rips out the BGR handling from imgconvert.c, and leaves everything to the gstreamer wrapping. Threre is some new code that prevents attempting to convert RGB to BGR using img_convert (we do that with caps only, which is way faster & stuff).
We quite definately need those conversions. What makes you think we don't need them? The peer element might only support one caps arrangement.
I think we do not need to rearrange the RGB bytes to BGR, but handle the RGB<->BGR stuff on the caps level, by setting red_mask and blue_mask appropriately. This works fine for me with patched pngdec (which supports only BGR and BGRA on its srcpad) and ximagesink (which supports only RGB555 on my system). Lets see... say, we have an E1 ! ffmpegcolorspace ! E2 pipeline, where E1's srcpad emits RGB24, and E2's sink accepts BGR24 only. The difference between the two is that red_mask and blue_mask are swapped. We can handle that on the caps level, no need to rearrange the bytes. This works the other way around too. And RGB32->BGR24 is the same as RGB32->RGB24 AND red_mask/blue_mask swapping at the caps level. All in all, I do not see the need to support rearranging RGB bytes to BGR, since the recommended way to access component bytes is to use the _mask properties. This patch removes the byte rearranging-foo, but the caps-level support is still in there.
But the element might not support the rearranged masks. If it did, passthrough would work fine.
If it does not support the rearranged masks, it probably doesn't want BGR, either. gstreamer/docs/random/mimetypes says: 24 and 32 bit RGB should always be specified as big endian, since any little endian format can be transformed into big endian by rearranging the color masks. 15 and 16 bit formats should generally have the same byte order as the CPU. So, if one element wants BGR, it should support the appropriate masks. Or, it might want 15/16bit format, which wasn't dealt with before, either. (RGB555 and RGB565 was dealt with, BGR555/BGR565 was not, as far as I could see) So far, I did not find any element that would be broken by this patch (mind you, I haven't tested all possible combinations). Furthermore, I think that if this breaks something, then the broken elements should be fixed to support the appropriate (rearranged) masks.
Hrm. Wait. I think I get the point... I'll think a bit more about this.
:). Yes, so the point is that if an element chooses to support only one set of masks, that's fine. ffmpegcolorspace is the bitch that's supposed to take care of all magic in between. :).
*sigh* Did I mention I hate colorspaces? And especially the horror that is ffmpegcolorspace? :) Anyways.. Here is my new plan: keep the BGR byte-swapping-foo, but teach ffmpegcolorspace that if it did the mask-swapping, it shouldn't do the byte-swapping, and vice versa. But before that, I revert my version of ffmpegcolorspace to current CVS, and see if I can reproduce the problem I see that way.
So what's the outcome?...
Didn't have time for this, sorry. I'll get back to it as school & work permit (which will most probably be early/mid february *sigh*)
Gergerly, what is the status of this bug? Is it still an issue or could this be closed?
Closing this bug report as no further information has been provided. Please feel free to reopen this bug if you can provide the information asked for. Thanks!