After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 164268 - [PATCH][ffmpegcolorspace] Fix BGR <-> RGB handling
[PATCH][ffmpegcolorspace] Fix BGR <-> RGB handling
Status: RESOLVED INCOMPLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: High normal
: NONE
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2005-01-16 18:35 UTC by Gergely Nagy
Modified: 2006-05-15 10:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (9.29 KB, patch)
2005-01-16 22:20 UTC, Gergely Nagy
reviewed Details | Review

Description Gergely Nagy 2005-01-16 18:35:57 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)
Comment 1 Gergely Nagy 2005-01-16 22:20:53 UTC
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).
Comment 2 Ronald Bultje 2005-01-17 08:55:22 UTC
We quite definately need those conversions. What makes you think we don't need
them? The peer element might only support one caps arrangement.
Comment 3 Gergely Nagy 2005-01-17 09:18:22 UTC
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.
Comment 4 Ronald Bultje 2005-01-17 09:45:11 UTC
But the element might not support the rearranged masks. If it did, passthrough
would work fine.
Comment 5 Gergely Nagy 2005-01-17 10:52:57 UTC
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.
Comment 6 Gergely Nagy 2005-01-17 11:03:42 UTC
Hrm. Wait. I think I get the point... I'll think a bit more about this.
Comment 7 Ronald Bultje 2005-01-17 11:07:38 UTC
:). 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. :).
Comment 8 Gergely Nagy 2005-01-17 11:23:49 UTC
*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.
Comment 9 Ronald Bultje 2005-01-25 17:54:13 UTC
So what's the outcome?...
Comment 10 Gergely Nagy 2005-01-25 17:59:58 UTC
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*)
Comment 11 Christian Fredrik Kalager Schaller 2005-12-07 11:27:51 UTC
Gergerly, what is the status of this bug? Is it still an issue or could this be
closed?
Comment 12 Tim-Philipp Müller 2006-05-15 10:01:16 UTC
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!