Can we not just ignore the alpha channel ?

You could either add a gray video format that has an alpha channel, or could convert inside the decoder to ARGB.

Thanks. Simplest approach is the ARGB idea.

Created attachment 353648 [details] [review]
openjpegdec: support grayscale with alpha

Review of attachment 353648 [details] [review]:

::: ext/openjpeg/gstopenjpegdec.c
@@ +386,3 @@
+      tmp[1] = off[0] + *data_in[0];
+      tmp[2] = tmp[1];
+      tmp[3] = tmp[1];

Is alpha always component 0 (data_in[0]) in openjpeg? And why do you output it as component 1 (tmp[1]), wouldn't that be RAGB or similar then?

Thanks for the review. On disk, alpha is invariably the last channel.
But, J2K has a colour definition box that can remap the channels, have
multiple alphas, or no alpha......

This would add another layer of complexity, but I could parse the box,
map the colours correctly, and deal with images with multiple alphas,
or four channels and no alpha. But, I have never seen such.

Also, there are palette images, but these are also rare.

Another issue to consider: there are two types of alpha - pre-multiplied and non-premultiplied. With pre-multiplied, the alpha has already been applied to the channel, and stored in the alpha channel. With non-pre-multiplied, alpha represents a floating point number between 0 and 1, which must then be multiplied with the colour channel to give the alpha representation.

pre-multiplied or not can be signalled with GstVideoMeta. Not sure what to do about the other things. What do you suggest?

I think at the moment, we can just limit support to single alpha channel for gray or RGB, and alpha must be the final channel. Hopefully that is not too heavy-handed :)

In the wild, my feeling is that 99% of all disk files are stored in this format.

I have updated the patch to do this checking, (I also corrected video format in a few places so that is consistently AXXX format, since all of the fill_frame methods expect output alpha to be stored in first component).

I have added a comment about how to determine pre-multiplied alpha, but don't know how to store this in GstVideoMeta.

Created attachment 353728 [details] [review]
Enforce single alpha channel.

Created attachment 353729 [details] [review]
Enforce single alpha channel for YUVA, RGBA and YA

Review of attachment 353729 [details] [review]:

::: ext/openjpeg/gstopenjpegdec.c
@@ +425,3 @@
+      tmp[1] = off[0] + (*data_in[0] << shift[0]);
+      tmp[2] = tmp[1];
+      tmp[3] = tmp[1];

Why are you generating something like RAGB here, i.e. alpha on the second component and all other components the same value?

@@ +888,2 @@
       if (image->numcomps == 4) {
+        /* alpha:  0 == colour channel, 1 == non-pre-multiplied alpha, 2 == pre-multiplied alpha */

This comment seems wrong for numcomps==4, and also above

@@ +903,3 @@
+          return GST_FLOW_NOT_NEGOTIATED;
+        }
+        if (alpha_channel != 3) {

If it's elsewhere, that seems to require only a minimal change to the conversion function or using e.g. BGRA instead of ABGR.

(In reply to Sebastian Dröge (slomo) from comment #13)
> Review of attachment 353729 [details] [review] [review]:
Thanks for the review

> 
> ::: ext/openjpeg/gstopenjpegdec.c
> @@ +425,3 @@
> +      tmp[1] = off[0] + (*data_in[0] << shift[0]);
> +      tmp[2] = tmp[1];
> +      tmp[3] = tmp[1];
> 
> Why are you generating something like RAGB here, i.e. alpha on the second
> component and all other components the same value?

I assume that alpha is always in the last component : component == 1. Since format is ARGB, and tmp[0] stores alpha in output, we need tmp[0] =  off[1] + (*data_in[1] << shift[1]);


> 
> @@ +888,2 @@
>        if (image->numcomps == 4) {
> +        /* alpha:  0 == colour channel, 1 == non-pre-multiplied alpha, 2 ==
> pre-multiplied alpha */
> 
> This comment seems wrong for numcomps==4, and also above

In the comment, alpha is referring to image->comps[c].alpha. If the value of image->comps[c].alpha is zero, then c is a colour channel. Otherwise, it is an alpha channel. I will make this clearer.


> 
> @@ +903,3 @@
> +          return GST_FLOW_NOT_NEGOTIATED;
> +        }
> +        if (alpha_channel != 3) {
> 
> If it's elsewhere, that seems to require only a minimal change to the
> conversion function or using e.g. BGRA instead of ABGR.

The code previously was assuming that alpha is always the last channel in the input; just making this assumption explicit :)  I don't think input frames have alpha in the first channel ?

For simplicity, can we not just assume alpha as last channel in input ? That's what the code was doing previously for RGBA, I'm just adding YA support....

Sure, that's fine, it just seems like I'm misunderstanding something with the patch :)

(In reply to Aaron Boxer from comment #14)
> (In reply to Sebastian Dröge (slomo) from comment #13)
> > Review of attachment 353729 [details] [review] [review] [review]:
> Thanks for the review
> 
> > 
> > ::: ext/openjpeg/gstopenjpegdec.c
> > @@ +425,3 @@
> > +      tmp[1] = off[0] + (*data_in[0] << shift[0]);
> > +      tmp[2] = tmp[1];
> > +      tmp[3] = tmp[1];
> > 
> > Why are you generating something like RAGB here, i.e. alpha on the second
> > component and all other components the same value?
> 
> I assume that alpha is always in the last component : component == 1. Since
> format is ARGB, and tmp[0] stores alpha in output, we need tmp[0] =  off[1]
> + (*data_in[1] << shift[1]);

But shouldn't that then be
tmp[0] = off[1] ...
tmp[1] = off[0] ...
tmp[2] = off[0] ...
tmp[3] = off[0] ...

I.e. alpha goes to alpha, grey value goes to R,G,B?

(In reply to Sebastian Dröge (slomo) from comment #16)
> Sure, that's fine, it just seems like I'm misunderstanding something with
> the patch :)

no problem :)


> 
> (In reply to Aaron Boxer from comment #14)
> > (In reply to Sebastian Dröge (slomo) from comment #13)
> > > Review of attachment 353729 [details] [review] [review] [review] [review]:
> > Thanks for the review
> > 
> > > 
> > > ::: ext/openjpeg/gstopenjpegdec.c
> > > @@ +425,3 @@
> > > +      tmp[1] = off[0] + (*data_in[0] << shift[0]);
> > > +      tmp[2] = tmp[1];
> > > +      tmp[3] = tmp[1];
> > > 
> > > Why are you generating something like RAGB here, i.e. alpha on the second
> > > component and all other components the same value?
> > 
> > I assume that alpha is always in the last component : component == 1. Since
> > format is ARGB, and tmp[0] stores alpha in output, we need tmp[0] =  off[1]
> > + (*data_in[1] << shift[1]);
> 
> But shouldn't that then be
> tmp[0] = off[1] ...
> tmp[1] = off[0] ...
> tmp[2] = off[0] ...
> tmp[3] = off[0] ...
> 
> I.e. alpha goes to alpha, grey value goes to R,G,B?

I am just trying to re-use the R value in the G and B channels. R == tmp[1]
Just save a little calculation :)

Created attachment 353814 [details] [review]
update comments on previous patch

So, I think the only missing piece in this patch is
how to signal that the opacity channel is pre-multiplied or not.

See GST_VIDEO_FLAG_PREMULTIPLIED_ALPHA. While this can be put into a GstVideoInfo, there's nothing that does anything meaningful with it for your case. It would have to be added to the caps or to the GstVideoMeta somehow to be useful.

Without such changes, only non-premultiplied alpha is supported currently.

Created attachment 354092 [details] [review]
Support alpha channel (pre-multiplied is treated as non-premultiplied, and user is warned in this case)

I am happy with this patch - it adds support for mono with alpha, fixes a few bugs with RGBA, and handles pre-multiplied alpha be warning the user and treating as non-pre-multiplied. As far as I know, most images these days do not use pre-multiplied alpha.

The patch also makes explicit the assumption in the current code that alpha is expected as the last channel in the input image. I believe this is the case for almost all J2K images in the wild.

Created attachment 354405 [details] [review]
Support alpha channel (pre-multiplied is treated as non-premultiplied, and user is warned in this case)

This patch is ready to change the world :) just needs merging.

commit 1883ac26b7d02724c11d4f4bad8698c4873b443d (HEAD -> master)
Author: Aaron Boxer <boxerab@gmail.com>
Date:   Mon Jun 12 23:36:05 2017 -0400

    openjpegdec: support grayscale with alpha channel
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783591

This breaks the build with older openjpeg it seems:

> gstopenjpegdec.c: In function ‘gst_openjpeg_dec_negotiate’:
> gstopenjpegdec.c:752:30: error: ‘opj_image_comp_t {aka struct opj_image_comp}’ has no member named ‘alpha’
>            if (image->comps[i].alpha) {
>                               ^

damn! Yes, openjpeg doesn't have the alpha member, I added it on my project.
So, will have to remove a lot of this code then. I will assume that 2 component
image is YA and 4 component image is RGBA.

Created attachment 355039 [details] [review]
support grayscale with alpha

Since we can't get alpha info from openjpeg, we assume that grayscale with alpha is in the format YA, where Y is the luminance channel, and A is the alpha channel.

commit 6d723303a28a6dbe7b40911885d50acea9099a67 (HEAD -> master)
Author: Aaron Boxer <boxerab@gmail.com>
Date:   Mon Jun 12 23:36:05 2017 -0400

    openjpegdec: support grayscale with alpha channel
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783591