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 767908 - jpeg2000parse: use enums for colorspace and sampling, rather than strings
jpeg2000parse: use enums for colorspace and sampling, rather than strings
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-21 11:46 UTC by Aaron Boxer
Modified: 2016-07-01 14:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use enums for colorspace and sampling (29.55 KB, patch)
2016-06-21 16:42 UTC, Aaron Boxer
none Details | Review
Use enums for colorspace and sampling (32.45 KB, patch)
2016-06-21 20:19 UTC, Aaron Boxer
none Details | Review
Use enums for colorspace and sampling (32.82 KB, patch)
2016-06-22 18:01 UTC, Aaron Boxer
none Details | Review
Use enums for colorspace and sampling (32.76 KB, patch)
2016-06-22 18:06 UTC, Aaron Boxer
none Details | Review
Use enums for colorspace and sampling (32.76 KB, patch)
2016-06-22 18:07 UTC, Aaron Boxer
none Details | Review
Use enums for colorspace and sampling (34.00 KB, patch)
2016-06-23 11:18 UTC, Aaron Boxer
committed Details | Review

Description Aaron Boxer 2016-06-21 11:46:32 UTC
Nicer to work with enums than with strings and strcmp
Comment 1 Sebastian Dröge (slomo) 2016-06-21 12:05:59 UTC
Initial patch here https://bugzilla.gnome.org/show_bug.cgi?id=767512#c45
Comment 2 Aaron Boxer 2016-06-21 14:38:21 UTC
Review from 767512:



::: ext/openjpeg/Makefile.am
@@ +1,3 @@
 plugin_LTLIBRARIES = libgstopenjpeg.la
 
+libgstopenjpeg_la_SOURCES = gstopenjpegdec.c gstopenjpegenc.c ../../gst/videoparsers/gstjpeg2000sampling.c gstopenjpeg.c

Why not let the openjpeg plugin link to libgstcodecparsers instead?

::: gst/videoparsers/gstjpeg2000sampling.c
@@ +22,3 @@
+
+
+static const gchar *GstRtpJ2kSamplingStrings[GST_RTP_J2K_UNKNOWN_SAMPLING] = {

You can leave the number of elements undefined

@@ +56,3 @@
+    case GST_RTP_J2K_NO_SAMPLING:
+    case GST_RTP_J2K_UNKNOWN_SAMPLING:
+      return 0;

NULL, not 0

@@ +86,3 @@
+    case GST_RTP_J2K_GRAYSCALE:
+      return "GRAYSCALE";
+      break;

Should get a default case

@@ +99,3 @@
+
+GstColorSpace
+gst_get_color_space (const gchar * colorspace_string)

g_return_val_if_fail (colorspace_string != NULL, UNKNOWN);

@@ +128,3 @@
+    case GST_GRAY:
+      return "GRAY";
+      break;

default case

@@ +130,3 @@
+      break;
+  }
+  return 0;

NULL

::: gst/videoparsers/gstjpeg2000sampling.h
@@ +53,3 @@
+  GST_RTP_J2K_RGBA,
+  GST_RTP_J2K_BGRA,
+  GST_RTP_J2K_YBRA,

YBRA was our addition, not from the standard, right? Also it should probably be YBRA4444

We should somehow mark our additions that are not in the RTP standard

@@ +60,3 @@
+  GST_RTP_J2K_GRAYSCALE,
+  GST_RTP_J2K_UNKNOWN_SAMPLING,
+  GST_RTP_J2K_NO_SAMPLING

What's the difference between no and unknown sampling?

For extensibility it would be better to put these two at the very front, you can then add new samplings in a backwards compatible way without having these two cases in the middle

@@ +78,3 @@
+  GST_GRAY,
+  GST_UNKNOWN_COLOR_SPACE,
+  GST_NO_COLOR_SPACE

Same here about the unknown ones, and please namespace them properly.

@@ +82,3 @@
+
+const gchar *gst_get_color_space_string (GstColorSpace colorspace);
+GstColorSpace gst_get_color_space (const gchar * colorspace_string);

These should be namespaced with j2k or similar. In general the namespacing should be consistent and this should probably go into codecparsers (and later pbutils/codecutils I guess)
Comment 3 Aaron Boxer 2016-06-21 14:39:41 UTC
(In reply to boxerab@gmail.com from comment #2)
> Review from 767512:
> 
> 
> 
> ::: ext/openjpeg/Makefile.am
> @@ +1,3 @@
>  plugin_LTLIBRARIES = libgstopenjpeg.la
>  
> +libgstopenjpeg_la_SOURCES = gstopenjpegdec.c gstopenjpegenc.c
> ../../gst/videoparsers/gstjpeg2000sampling.c gstopenjpeg.c
> 
> Why not let the openjpeg plugin link to libgstcodecparsers instead?
> 

Thanks. I tried that, using

libgstopenjpeg_la_LIBADD = \
	$(top_builddir)/gst-libs/gst/codecparsers/libgstcodecparsers-$(GST_API_VERSION).la \

but I get a runtime error about missing symbols.
Comment 4 Sebastian Dröge (slomo) 2016-06-21 14:51:49 UTC
You have to move the symbols to libgstcodecparsers first, currently you have them in the videoparsers plugin :)
Comment 5 Aaron Boxer 2016-06-21 14:55:00 UTC
ahhh, got it, thanks :)
Comment 6 Aaron Boxer 2016-06-21 14:55:29 UTC
should I move the jpeg2000 parser as well ?
Comment 7 Sebastian Dröge (slomo) 2016-06-21 15:00:38 UTC
Unless you write some parsing API around it, similar to the JPEG one, no.
Comment 8 Aaron Boxer 2016-06-21 15:18:26 UTC
ok, thanks.
Comment 9 Aaron Boxer 2016-06-21 16:42:37 UTC
Created attachment 330147 [details] [review]
Use enums for colorspace and sampling
Comment 10 Sebastian Dröge (slomo) 2016-06-21 18:25:15 UTC
Review of attachment 330147 [details] [review]:

Incomplete review, but all other things go along the same line

::: ext/openjpeg/gstopenjpegdec.h
@@ +25,3 @@
 #include <gst/gst.h>
 #include <gst/video/video.h>
+#include "../../gst-libs/gst/codecparsers/gstjpeg2000sampling.h"

<gst/codecparsers/...>

::: ext/openjpeg/gstopenjpegenc.c
@@ +118,3 @@
         "num-components = (int) [1, 4], "
         GST_RTP_J2K_SAMPLING_LIST ","
+        GST_JPEG2000_SAMPLING_COLORSPACE_LIST "; "

This might be worth moving to the library too... but the naming should all be more consistent. GST_JPEG2000_* maybe, and call it in the style of GST_VIDEO_FORMATS_ALL

::: gst-libs/gst/codecparsers/gstjpeg2000sampling.c
@@ +21,3 @@
+#include "gstjpeg2000sampling.h"
+
+static const gchar *GstRtpJ2kSamplingStrings[] = {

snake_case_for_variable_names :)

@@ +37,3 @@
+gst_rtp_j2k_get_sampling (const gchar * sampling_string)
+{
+  GstRtpJ2kSampling i;

g_return_val_if_fail() guards for all the non-static functions

::: gst-libs/gst/codecparsers/gstjpeg2000sampling.h
@@ +26,3 @@
+
+/**********************************************************************************************/
+/* Sampling values from RF 5371 for JPEG 2000 over RTP : https://datatracker.ietf.org/doc/rfc5371/C

Make this a proper gtk-doc documentation comment, see various headers in gstreamer core for examples

@@ +61,3 @@
+  GST_RTP_J2K_GRAYSCALE,
+  GST_RTP_J2K_YBRA4444_EXT,
+  GST_RTP_J2K_NO_SAMPLING

No sampling should move to the very beginning

@@ +65,3 @@
+
+
+#define GST_RTP_J2K_SAMPLING_LIST "sampling = (string) {\"RGB\", \"BGR\", \"RGBA\", \"BGRA\", \"YCbCr-4:4:4\", \"YCbCr-4:2:2\", \"YCbCr-4:2:0\", \"YCbCr-4:1:1\", \"GRAYSCALE\" , \"YCbCrA\"}"

YCbCrA should probably get -4:4:4:4 added

@@ +86,3 @@
+} GstJPEG2000SamplingColorSpace;
+
+const gchar *gst_get_color_space_string (GstJPEG2000SamplingColorSpace

This needs to be namespaced properly. gst_jpeg2000_*

@@ +88,3 @@
+const gchar *gst_get_color_space_string (GstJPEG2000SamplingColorSpace
+    colorspace);
+GstJPEG2000SamplingColorSpace gst_get_color_space (const gchar *

What we usually do for these two kinds of functions is from_string / to_string.

@@ +119,3 @@
+#define GST_J2K_SIZE_OF_MARKER  	4
+#define GST_J2K_SIZE_OF_BOX_ID  	4
+#define GST_J2K_SIZE_OF_BOX_LEN		4

JPEG2000 instead of J2K. All needs to be made more consistent :)
Comment 11 Aaron Boxer 2016-06-21 19:07:15 UTC
> 
> ::: ext/openjpeg/gstopenjpegenc.c
> @@ +118,3 @@
>          "num-components = (int) [1, 4], "
>          GST_RTP_J2K_SAMPLING_LIST ","
> +        GST_JPEG2000_SAMPLING_COLORSPACE_LIST "; "
> 
> This might be worth moving to the library too... but the naming should all
> be more consistent. GST_JPEG2000_* maybe, and call it in the style of
> GST_VIDEO_FORMATS_ALL

Thanks. You mentioned a plan to eventually replace colorspace with the sampling field. If that's the case, should we not be using something more general than JPEG2000 to namespace the sampling strings?
Comment 12 Aaron Boxer 2016-06-21 19:17:00 UTC
For now, I will use GST_JPEG2000_PARSE for the namespace, and camel-case on variable names: GstJPEG2000Parse
Comment 13 Aaron Boxer 2016-06-21 20:19:41 UTC
Created attachment 330170 [details] [review]
Use enums for colorspace and sampling

Thanks for the review. I think I've addressed most of the issues you raised.
Comment 14 Sebastian Dröge (slomo) 2016-06-22 07:02:43 UTC
Review of attachment 330170 [details] [review]:

I wanted to get rid of the colorspace field in the JPEG2000 caps (for GStreamer 2.0), not in general :) In general I'd prefer to get rid of the fourccs in the video/x-raw caps and replace them with something more meaningful, but that's unrelated.

::: gst-libs/gst/codecparsers/gstjpeg2000sampling.c
@@ +21,3 @@
+#include "gstjpeg2000sampling.h"
+
+static const gchar *GstJPEG2000SamplingStrings[] = {

snake_case variable names

@@ +51,3 @@
+gst_jpeg2000_get_sampling_string (GstJPEG2000Sampling sampling)
+{
+  if (sampling > 0 && sampling <= sizeof (GstJPEG2000SamplingStrings))

This should probably also be a g_return_val_if_fail() instead

::: gst-libs/gst/codecparsers/gstjpeg2000sampling.h
@@ +53,3 @@
+  GST_JPEG2000_YBR410,
+  GST_JPEG2000_GRAYSCALE,
+  GST_JPEG2000_YBRA4444_EXT

GST_JPEG2000_SAMPLING_XXX

@@ +59,3 @@
+#define GST_JPEG2000_SAMPLING_LIST "sampling = (string) {\"RGB\", \"BGR\", \"RGBA\", \"BGRA\", \"YCbCr-4:4:4\", \"YCbCr-4:2:2\", \"YCbCr-4:2:0\", \"YCbCr-4:1:1\", \"GRAYSCALE\" , \"YCbCrA-4:4:4:4\"}"
+
+const gchar *gst_jpeg2000_get_sampling_string (GstJPEG2000Sampling sampling);

gst_jpeg2000_sampling_to_string()

@@ +60,3 @@
+
+const gchar *gst_jpeg2000_get_sampling_string (GstJPEG2000Sampling sampling);
+GstJPEG2000Sampling gst_jpeg2000_get_sampling (const gchar * sampling_string);

gst_jpeg2000_sampling_from_string()

@@ +75,3 @@
+  GST_JPEG2000_SAMPLING_RGB,
+  GST_JPEG2000_SAMPLING_YUV,
+  GST_JPEG2000_SAMPLING_GRAY

GST_JPEG2000_SAMPLING_COLORSPACE_XXX but it seems like GstJPEG2000Colorspace / GST_JPEG2000_COLORSPACE_XXX is better

@@ +80,3 @@
+const gchar *gst_jpeg2000_get_color_space_string (GstJPEG2000SamplingColorSpace
+    colorspace);
+GstJPEG2000SamplingColorSpace gst_jpeg2000_get_color_space (const gchar *

to_string() and from_string()

@@ +83,3 @@
+    colorspace_string);
+
+#define GST_JPEG2000_SAMPLING_COLORSPACE_LIST "colorspace = (string) { \"sRGB\", \"sYUV\", \"GRAY\" }"

No SAMPLING here
Comment 15 Aaron Boxer 2016-06-22 18:01:38 UTC
Created attachment 330205 [details] [review]
Use enums for colorspace and sampling

Changes from latest review.
Comment 16 Aaron Boxer 2016-06-22 18:06:31 UTC
Created attachment 330206 [details] [review]
Use enums for colorspace and sampling
Comment 17 Aaron Boxer 2016-06-22 18:07:10 UTC
Created attachment 330207 [details] [review]
Use enums for colorspace and sampling
Comment 18 Aaron Boxer 2016-06-22 18:12:32 UTC
(In reply to Sebastian Dröge (slomo) from comment #14)
> Review of attachment 330170 [details] [review] [review]:
> 
> I wanted to get rid of the colorspace field in the JPEG2000 caps (for
> GStreamer 2.0), not in general :) In general I'd prefer to get rid of the
> fourccs in the video/x-raw caps and replace them with something more
> meaningful, but that's unrelated.

Thanks. I do think it would be beneficial to use enums for general colorspace fields, and to be able to re-use a #define with a list of possible color spaces, for the caps. This would make it just a bit easier for creation of new plugins. My 2 cents :)
Comment 19 Sebastian Dröge (slomo) 2016-06-23 07:05:58 UTC
Review of attachment 330207 [details] [review]:

Looks good to me now except for one small thing :)

::: gst-libs/gst/codecparsers/gstjpeg2000sampling.h
@@ +26,3 @@
+
+/**
+* Sampling values from RF 5371 for JPEG 2000 over RTP : https://datatracker.ietf.org/doc/rfc5371/C

Please make this a proper gtk-doc comment, and also document all other public functions and enums. Functions in .c, everything else in .h
Comment 20 Aaron Boxer 2016-06-23 11:18:21 UTC
Created attachment 330247 [details] [review]
Use enums for colorspace and sampling

added documentation
Comment 21 Sebastian Dröge (slomo) 2016-06-24 08:23:48 UTC
commit e7e6a3579d7ec3b61c80040c679c48c216fb7065
Author: Aaron Boxer <boxerab@gmail.com>
Date:   Tue Jun 21 12:41:46 2016 -0400

    jpeg2000parse: use enums for colorspace and sampling, rather than strings
    
    Also, move gstjpeg2000sampling to codecparsers project
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767908
Comment 22 Tim-Philipp Müller 2016-06-24 08:42:30 UTC
commit ab2281be0f47838f5665652fad58f38b35ca8bf4
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Jun 24 09:41:18 2016 +0100

    openjpeg: fix more broken includes

commit cb7b6e7a90c785f86f67e7cf503f025257dc80c2
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Jun 24 09:39:00 2016 +0100

    videoparsers: fix broken include
Comment 23 Tim-Philipp Müller 2016-07-01 12:03:48 UTC
Aaron: please note that sizeof(array) is not the same as G_N_ELEMENTS(array) :)

commit e92894e4ab344f8c0fa8ea69c231eaaee6b83ac8
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Jul 1 12:20:14 2016 +0100

    codecparsers: jpeg2000: don't allow 0 value for array access with i-1

commit 1462ac7cdfb0e5202a299bcb8af3ac93d2a621a9
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Jul 1 12:15:28 2016 +0100

    codecparsers: jpeg2000: fix possible out-of-bounds array access
    
    sizeof(array) != G_N_ELEMENTS(array).
    
    CID 1362900
Comment 24 Aaron Boxer 2016-07-01 14:37:27 UTC
(In reply to Tim-Philipp Müller from comment #23)
> Aaron: please note that sizeof(array) is not the same as G_N_ELEMENTS(array)

omg, yes. :) Nice catch.