GNOME Bugzilla – Bug 767908
jpeg2000parse: use enums for colorspace and sampling, rather than strings
Last modified: 2016-07-01 14:37:27 UTC
Nicer to work with enums than with strings and strcmp
Initial patch here https://bugzilla.gnome.org/show_bug.cgi?id=767512#c45
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)
(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.
You have to move the symbols to libgstcodecparsers first, currently you have them in the videoparsers plugin :)
ahhh, got it, thanks :)
should I move the jpeg2000 parser as well ?
Unless you write some parsing API around it, similar to the JPEG one, no.
ok, thanks.
Created attachment 330147 [details] [review] Use enums for colorspace and sampling
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 :)
> > ::: 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?
For now, I will use GST_JPEG2000_PARSE for the namespace, and camel-case on variable names: GstJPEG2000Parse
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.
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
Created attachment 330205 [details] [review] Use enums for colorspace and sampling Changes from latest review.
Created attachment 330206 [details] [review] Use enums for colorspace and sampling
Created attachment 330207 [details] [review] Use enums for colorspace and sampling
(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 :)
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
Created attachment 330247 [details] [review] Use enums for colorspace and sampling added documentation
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
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
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
(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.