[Bug 767908] jpeg2000parse: use enums for colorspace and sampling, rather than strings

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=767908

--- Comment #2 from boxerab at gmail.com <boxerab at gmail.com> ---
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)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.