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 506132 - Review of changes in video/video.h
Review of changes in video/video.h
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal blocker
: 0.10.16
Assigned To: David Schleef
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2007-12-28 19:53 UTC by David Schleef
Modified: 2008-01-14 18:21 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description David Schleef 2007-12-28 19:53:28 UTC
Please comment on recent additions to video.h.
Comment 1 Tim-Philipp Müller 2008-01-03 15:57:31 UTC
These might also be nice to have:

 gboolean   gst_video_format_is_packed (GstVideoFormat format);

 int        gst_video_format_get_pixel_offset (GstVideoFormat format,
                int component, int width, int height, int x, int y);

The latter would be a utility function for

  plane_offset + y*foo + x*bar.

(not sure about the name of the latter, maybe _get_offset(), or _get_position_offset() would be better).
Comment 2 Tim-Philipp Müller 2008-01-08 17:44:50 UTC
Random addendum: it's sure nice to finally have all this stuff in libgstvideo, but it's all very tedious to use. The function names are very long, so are the video format constants, and you have to do lots of calls to get the strides/offsets etc. The code just doesn't look nice. I'd prefer a function to which I can just pass format/width/height/pixeldatapointer and which returns a struct with all the stuff needed (offset by pixeldatapointer).
Comment 3 David Schleef 2008-01-08 19:10:05 UTC
I tried it both ways, and concluded this was better.  Using a struct ends up with longer, less readable lines:

  pixelstride = format->components[i].pixelstride;

  pixelstride = gst_format_get_pixel_stride (format, i);

Comment 4 Jan Schmidt 2008-01-12 17:01:25 UTC
Why not just format->strides[i] ?
Comment 5 David Schleef 2008-01-12 23:19:38 UTC
The difference between using functions vs. structures at this level is pretty much a matter of style.  However, my reasoning here is mostly:

 - Functions are easier to extend: just add another function.  Structures, you have to worry about ABI issues.

 - Functions are simple and obviously named.  IMO, it's much harder for a user to understand the fields in a structure than the return value of a function.  This is especially true when some of the structure fields are irrelevant for certain formats.

 - With structures, you have to worry about refcounting/freeing

 - Functions map very easily into other languages.  Structures somewhat, but it's more difficult and takes effort.

Comment 6 Jan Schmidt 2008-01-14 09:58:38 UTC
All good arguments :)

I read the code on the weekend, I'm happy with the API - can we close the bug, or would someone like step up to add the other API Tim suggested (It might not make today's pre-release, but there's another window for one on Thursday)?

Comment 7 Tim-Philipp Müller 2008-01-14 14:29:13 UTC
I'm fine with it as it is. Dave's argument for functions makes sense. Convenience functions can always be added later, same goes for _is_packed(). I don't have an urgent need for either right now.
Comment 8 Jan Schmidt 2008-01-14 18:21:02 UTC
OK, ta. Marking as closed.