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 735032 - zebrastripe: Refactoring of common code
zebrastripe: Refactoring of common code
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
Other All
: Normal minor
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2014-08-19 05:09 UTC by Vineeth
Modified: 2014-09-01 14:11 UTC
See Also:
GNOME target: ---
GNOME version: ---

Refactored ZebraStripe element (3.88 KB, patch)
2014-08-19 05:09 UTC, Vineeth
reviewed Details | Review
Made changes as per the review comments. (3.87 KB, patch)
2014-08-26 04:15 UTC, Vineeth
reviewed Details | Review
Patch with proper commit comment (4.19 KB, patch)
2014-09-01 02:57 UTC, Vineeth
committed Details | Review

Description Vineeth 2014-08-19 05:09:36 UTC
Created attachment 283859 [details] [review]
Refactored ZebraStripe element

In GstZebraStripe element,
there are three functions

which does the same functionality except for the offset values
The code is duplicated in all 3 functions.

Hence moving the code to a common place and just changing the offset values based on the CAPS.
Comment 1 Thiago Sousa Santos 2014-08-25 17:50:36 UTC
Review of attachment 283859 [details] [review]:

Nice catch, some comments for more readability below.

::: gst/videofilters/gstzebrastripe.c
@@ +221,3 @@
     case GST_VIDEO_FORMAT_Y444:
     case GST_VIDEO_FORMAT_Y42B:
+      data_a = 1;

GST_VIDEO_FORMAT_INFO_PSTRIDE can be used to get those values for data_a

@@ +241,3 @@
+        (guint8 *) frame->data[0] + frame->info.stride[0] * j + offset;
+    for (i = 0; i < width; i++) {
+      if (data[data_a * i + data_b] >= threshold) {

How about using more significative names for data_a and data_b.

data_a seems to be the pixel stride and data_b the index of the Y inside the pixel. Maybe something like pixel_stride and y_position or similar.
Comment 2 Vineeth 2014-08-26 04:15:58 UTC
Created attachment 284475 [details] [review]
Made changes as per the review comments.

Hi Thiago,

Thanks for the review
Have made changes as per the review comments.
Please review the same.
Comment 3 Thiago Sousa Santos 2014-08-28 13:45:32 UTC
Review of attachment 284475 [details] [review]:

Code looks good now, could you please elaborate a better commit message? If you look at other commits you will figure out the pattern used in gstreamer. It is something like:

elementname: short title

long description (multiple lines)

bugzilla bug url (on the last line)
Comment 4 Vineeth 2014-09-01 02:57:34 UTC
Created attachment 284968 [details] [review]
Patch with proper commit comment

Edited the commit message as per the norm followed for Gstreamer.
Comment 5 Thiago Sousa Santos 2014-09-01 14:09:21 UTC
Thanks for the update.

commit 114ee3355ad6db075c230f26ed92da84953a6def
Author: Vineeth T M <>
Date:   Mon Sep 1 08:24:57 2014 +0530

    zebrastripe: Refactor to remove duplicate code
    all above 3 functions do the same functionality except for offset and pixel stride.
    Hence moving the functionality to a single funtion.