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 793011 - GstAdapter: Improve documentation for gst_adapter_available()
GstAdapter: Improve documentation for gst_adapter_available()
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: documentation
unspecified
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-01-29 19:07 UTC by Alicia Boya García
Modified: 2018-11-03 11:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GstAdapter: Improve documentation for gst_adapter_available() (1.98 KB, patch)
2018-01-29 19:07 UTC, Alicia Boya García
reviewed Details | Review

Description Alicia Boya García 2018-01-29 19:07:34 UTC
When I first read the documentation of gst_adapter_available() and
gst_adapter_available_free() I got quite confused as it seemed that both
performed the same purpose but one was slower than the other.

I shared it with other people and found they also arrived at the same
wrong conclusion.

Hopefully this patch will make the actual purpose clearer.
Comment 1 Alicia Boya García 2018-01-29 19:07:39 UTC
Created attachment 367594 [details] [review]
GstAdapter: Improve documentation for gst_adapter_available()

When I first read the documentation of gst_adapter_available() and
gst_adapter_available_free() I got quite confused as it seemed that both
performed the same purpose but one was slower than the other.

I shared it with other people and found they also arrived at the same
wrong conclusion.

Hopefully this patch will make the actual purpose clearer.
Comment 2 Alicia Boya García 2018-01-29 19:10:28 UTC
s/gst_adapter_available_free/gst_adapter_available_fast/
Comment 3 Thibault Saunier 2018-01-29 19:20:37 UTC
Review of attachment 367594 [details] [review]:

I agree the naming of the `available_fast` variant is confusing, I would have explicitely explained in the doc that the `fast` variant is not about the fastness of the function itself but about the amout of data that can be accessed fast (without extra memcopy).
Comment 4 Tim-Philipp Müller 2018-01-29 19:56:07 UTC
Comment on attachment 367594 [details] [review]
GstAdapter: Improve documentation for gst_adapter_available()

>When I first read the documentation of gst_adapter_available() and
>gst_adapter_available_free() I got quite confused as it seemed that both
>performed the same purpose but one was slower than the other.
>
>I shared it with other people and found they also arrived at the same
>wrong conclusion.
>
>Hopefully this patch will make the actual purpose clearer.

I find it hard to believe that anyone could be confused after reading the actual description of both functions and return values side by side ("Returns: number of bytes available in adapter" vs "Returns: number of bytes that are available in adapter without expensive operations"), but it's always possible to make things clearer of course :)


>diff --git a/libs/gst/base/gstadapter.c b/libs/gst/base/gstadapter.c
>index e3dff288f..d4c7b8f5e 100644
>--- a/libs/gst/base/gstadapter.c
>+++ b/libs/gst/base/gstadapter.c
>@@ -1313,6 +1313,10 @@ gst_adapter_get_buffer_list (GstAdapter * adapter, gsize nbytes)
>  * value that can be supplied to gst_adapter_map() without that function
>  * returning %NULL.
>  *
>+ * Calling gst_adapter_map() with the amount of bytes returned by this function
>+ * may require expensive operations (like copying the data into a temporary
>+ * buffer) in some cases.
>+ *

How about we also explicitly mention the _fast() variant here for comparison then?



>@@ -1327,8 +1331,11 @@ gst_adapter_available (GstAdapter * adapter)
>  * gst_adapter_available_fast:
>  * @adapter: a #GstAdapter
>  *
>- * Gets the maximum number of bytes that are immediately available without
>- * requiring any expensive operations (like copying the data into a
>+ * Gets the maximum number of bytes that can be retrieved in a single map
>+ * operation without merging buffers.

Here I think the previous version is actually clearer. You seem very focused on gst_adapter_map() here, but a user of the adapter API might access the data in the adapter in a number of ways, and the existing sentence is more generic. It's not clear to me that talking about "a single map" here makes things much clearer. It's not the mapping that's the problem, it's the copying/merging. And the term "map" is overloaded from buffers and memories as well as the adapter peeking function which probably shouldn't have been called _map() but we are where we are :)


>+ * Calling gst_adapter_map() with the amount of bytes returned by this function
>+ * will never require any expensive operations (like copying the data into a
>  * temporary buffer).
>  *
>  * Returns: number of bytes that are available in @adapter without expensive
Comment 5 Alicia Boya García 2018-01-29 20:27:48 UTC
(In reply to Tim-Philipp Müller from comment #4)
> I find it hard to believe that anyone could be confused after reading the
> actual description of both functions and return values side by side
> ("Returns: number of bytes available in adapter" vs "Returns: number of
> bytes that are available in adapter without expensive operations"), but it's
> always possible to make things clearer of course :)

As Sebastian always says... everything is obvious once you know it.

I changed the first phrase to say "that can be retrieved in a single map operation" instead of "immediately available" because it's not necessarily clear what immediately would mean there. Hopefully the next phrase makes it clear. Do you have a suggestion on a clearer phrasing?
Comment 6 GStreamer system administrator 2018-11-03 11:04:00 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-docs/issues/12.