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 765600 - gst_memory_map: delegate logging traces to subclass on failure
gst_memory_map: delegate logging traces to subclass on failure
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 755072
 
 
Reported: 2016-04-26 12:10 UTC by Julien Isorce
Modified: 2016-11-21 21:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
memory: add gst_memory_try_map (9.60 KB, patch)
2016-06-21 14:31 UTC, Julien Isorce
none Details | Review
memory: remove GST_ERROR on map failure (6.20 KB, patch)
2016-06-23 08:11 UTC, Julien Isorce
none Details | Review
memory: log with GST_INFO instead GST_ERROR when subclass map failed. (6.21 KB, patch)
2016-10-07 14:18 UTC, Julien Isorce
committed Details | Review
gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied (1.26 KB, patch)
2016-10-07 14:30 UTC, Julien Isorce
none Details | Review
gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied (1.52 KB, patch)
2016-11-17 21:18 UTC, Julien Isorce
committed Details | Review

Description Julien Isorce 2016-04-26 12:10:01 UTC
Currently gst_memory_map always outputs a GST_ERROR on alocator->mem_map failutre: https://cgit.freedesktop.org/gstreamer/gstreamer/tree/gst/gstmemory.c#n324

There is at least one situation where we would like to use gst_memory_map as a try function: dmabuf.
Indeed we need to check at runtime if the given dmabuf is mappable and try another caps if not. So even if it fails it would be part of the caps negotiation phase so no need a GST_ERROR: https://github.com/CapOM/gstreamer-vaapi/commit/2a55a5fc774eb8bba68094ba231f06b8b07b3969#diff-bec9172c05be682d331c790d8a50ba95R195

Other solution would be to add a new GstMapFlags that would silent the error.
Comment 1 Sebastian Dröge (slomo) 2016-04-26 13:11:56 UTC
I like the idea of a new function, but it should also come in combination with gst_buffer_try_map(), try_map_range(), etc.

It seems cleaner than a mapping flag
Comment 2 Julien Isorce 2016-06-20 08:52:55 UTC
Ok I'll have a go this week I think.
Comment 3 Julien Isorce 2016-06-21 14:31:34 UTC
Created attachment 330143 [details] [review]
memory: add gst_memory_try_map

Please review, I'll add "gst_buffer_try_map" if that first patch is going the right way. Thx.
Comment 4 Sebastian Dröge (slomo) 2016-06-21 14:55:55 UTC
Review of attachment 330143 [details] [review]:

::: gst/gstmemory.c
@@ +343,3 @@
+ * @flags: mapping flags
+ *
+ * Same as gst_memory_map() except that it does not log an error on failure.

Maybe we shouldn't make this too explicit. We might want to add a vfunc for that to GstMemory later that does things differently for example. That it just does the same except for the error logging seems not too useful.

Not sure how to write that though

@@ +344,3 @@
+ *
+ * Same as gst_memory_map() except that it does not log an error on failure.
+ */

Since: 1.10
Comment 5 Julien Isorce 2016-06-21 15:14:29 UTC
(In reply to Sebastian Dröge (slomo) from comment #4)
> Review of attachment 330143 [details] [review] [review]:
> 
> ::: gst/gstmemory.c
> @@ +343,3 @@
> + * @flags: mapping flags
> + *
> + * Same as gst_memory_map() except that it does not log an error on failure.
> 
> Maybe we shouldn't make this too explicit. We might want to add a vfunc for
> that to GstMemory later that does things differently for example. That it
> just does the same except for the error logging seems not too useful.

Thx for the comments, do you mean like allocator->mem_map_full and allocator->mem_map it should be a mem_try_map ? Because I do not see much value of this since the underlying impl can already do what it prefers.

Or do you mean calling an hypothetic allocator->mem_on_map_fails instead of GST_CAT_ERROR in current gst_memory_map ?

Other way could be to pass a function pointer in parameter of gst_memory_try_map and calling that on error, and fallback to default if NULL.
Comment 6 Sebastian Dröge (slomo) 2016-06-22 06:43:38 UTC
I don't really know, it's all a bit annoying really to just do this for not doing a GST_ERROR() :)
Comment 7 Julien Isorce 2016-06-22 06:58:16 UTC
Yes it is annoying :) I was thinking that I could just move the GST_ERROR to the system memory so it defers responsibility to the underlying impls. For gst_dmabuf_allocator It will not log any error on map failure. So no need try map. Or comming back to the idea of a flag.
Comment 8 Sebastian Dröge (slomo) 2016-06-22 07:15:02 UTC
system memory can't really fail to map, unless you e.g. try to map it writable twice. Why is it so bad to have the GST_ERROR() btw? Would a GST_WARNING() be better? It's not really an error per se, it depends on why it fails (and e.g. mapping because of double-mapping and things like that seem like they should be errors).
Comment 9 Julien Isorce 2016-06-22 08:07:35 UTC
It is bad because soon every time one will run a pipeline involving gstvaapidecodebin he will see 2 GST_ERROR (buffer pool size). Indeed on going work from Victor here https://github.com/ceyusa/gstreamer-vaapi/commits/755072 will always try to use dmabuf. That is a consensus we came to with Benjamin, Nicolas, Victor and myself.
I personally can live with that but it is confusing for the user since it is a non-failure execution path (platform limitation or protected memory).

Indeed for system memory the map never fails. Double mapping is only a GST_DEBUG log trace.
Also I grep for "map_full (GstMemory" and "map (GstMemory". Only one occurrence in -base, gst_fd_mem_map and it already logs a GST_ERROR before returning NULL. (returning NULL from the vfunc map or map_full it the one that leads to the GST_ERROR in gst_memory_map call).

In -bad, some do GST_ERROR, some not. So for some it leads to have twice GST_ERROR (from parent and from impl). (having a GST_WARNING would not help I think)

I can remove the GST_ERROR from gst_memory_map, and add GST_ERROR in -bad for remaining memories that do not log it. (And why not adding G_GNUC_WARN_UNUSED_RESULT in gst_memory_map declaration ? I can grep places where the return is not used and add missing checks)
Comment 10 Nicolas Dufresne (ndufresne) 2016-06-22 12:39:56 UTC
I think I would prefer moving that to INFO (or any non-alarming level), but only for the one we know will fail due to trial. Adding more API is not so tempting.
Comment 11 Julien Isorce 2016-06-22 12:54:57 UTC
ERROR               fdmemory gstfdmemory.c:114:gpointer gst_fd_mem_map(GstMemory *, gsize, GstMapFlags): 0x7f92401d6b10: fd 15: mmap failed: ...
ERROR             GST_MEMORY gstmemory.c:324:gboolean gst_memory_map(GstMemory *, GstMapInfo *, GstMapFlags): mem 0x7f92401d6b10: subclass map failed

Could you precise your idea, i.e. which one to move to GST_INFO and what about the other one ?
Comment 12 Nicolas Dufresne (ndufresne) 2016-06-22 12:59:23 UTC
To be fair, I'd remove the "subclass map failed", this will always be redundant. Would be nice to check that all subclass do trace something though. And the FD one can probably be reduced to info. In 2.0, we should probably consider using a GError.

Any opinion on changing this error into something less noisy ?
Comment 13 Julien Isorce 2016-06-23 08:11:07 UTC
Created attachment 330237 [details] [review]
memory: remove GST_ERROR on map failure
Comment 14 Julien Isorce 2016-06-23 08:16:35 UTC
I plan to add traces in various subclasses when it is not already the case. It seems that for protected areas mmap will just return NULL so in that case gst_fd_mem_map does not log any error. When it returns MAP_FAILED, we can log GST_ERROR or GST_INFO depending on the "errno" value.
Comment 15 Nicolas Dufresne (ndufresne) 2016-06-26 11:22:36 UTC
(In reply to Julien Isorce from comment #14)
> I plan to add traces in various subclasses when it is not already the case.
> It seems that for protected areas mmap will just return NULL so in that case
> gst_fd_mem_map does not log any error. When it returns MAP_FAILED, we can
> log GST_ERROR or GST_INFO depending on the "errno" value.

Seems like a bad kernel implementation. Should fail with ENOPERM imh, as this is what everything else return in similar situations.
Comment 16 Julien Isorce 2016-10-07 14:18:20 UTC
Created attachment 337175 [details] [review]
memory: log with GST_INFO instead GST_ERROR when subclass map failed.

Let's keep at least a GST_INFO.
Comment 17 Julien Isorce 2016-10-07 14:30:18 UTC
Created attachment 337176 [details] [review]
gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied

Do not log error if EACCES (permission denied). Though I can see in libdrm that an ioctl call is done before mmap (DRM_NOUVEAU_GEM_CPU_PREP / DRM_RADEON_GEM_MMAP / DRM_I915_GEM_MMAP_GTT)
Comment 18 Tim-Philipp Müller 2016-11-11 18:30:47 UTC
Comment on attachment 337176 [details] [review]
gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied

>--- a/gst-libs/gst/allocators/gstfdmemory.c
>+++ b/gst-libs/gst/allocators/gstfdmemory.c
>@@ -110,9 +110,20 @@ gst_fd_mem_map (GstMemory * gmem, gsize maxsize, GstMapFlags flags)
> 
>     mem->data = mmap (0, gmem->maxsize, prot, flags, mem->fd, 0);
>     if (mem->data == MAP_FAILED) {
>+      GString *msg = g_string_new (NULL);
>       mem->data = NULL;
>-      GST_ERROR ("%p: fd %d: mmap failed: %s", mem, mem->fd,
>+
>+      g_string_printf (msg, "%p: fd %d: mmap failed: %s", mem, mem->fd,
>           g_strerror (errno));
>+      switch (errno) {
>+        case EACCES:
>+          GST_INFO ("%s", msg->str);
>+          break;
>+        default:
>+          GST_ERROR ("%s", msg->str);
>+      }
>+
>+      g_string_free (msg, TRUE);

We're now allocating, printf-ing and freeing a GString (and various helper allocs used inside g_string_printf) even if debug logging is completely disabled or not active, this seems suboptimal to me :)

Is there any reason not to just duplicate the statement for GST_INFO and GST_ERROR ?

If you *really* want to avoid this you can also use GST_CAT_LEVEL_LOG() and specify the level there.
Comment 19 Tim-Philipp Müller 2016-11-11 18:32:09 UTC
Comment on attachment 337175 [details] [review]
memory: log with GST_INFO instead GST_ERROR when subclass map failed.

Anything left to do here?
Comment 20 Julien Isorce 2016-11-15 11:48:30 UTC
(In reply to Tim-Philipp Müller from comment #18)
> Comment on attachment 337176 [details] [review] [review]
> gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied
> 
> >--- a/gst-libs/gst/allocators/gstfdmemory.c
> >+++ b/gst-libs/gst/allocators/gstfdmemory.c
> >@@ -110,9 +110,20 @@ gst_fd_mem_map (GstMemory * gmem, gsize maxsize, GstMapFlags flags)
> > 
> >     mem->data = mmap (0, gmem->maxsize, prot, flags, mem->fd, 0);
> >     if (mem->data == MAP_FAILED) {
> >+      GString *msg = g_string_new (NULL);
> >       mem->data = NULL;
> >-      GST_ERROR ("%p: fd %d: mmap failed: %s", mem, mem->fd,
> >+
> >+      g_string_printf (msg, "%p: fd %d: mmap failed: %s", mem, mem->fd,
> >           g_strerror (errno));
> >+      switch (errno) {
> >+        case EACCES:
> >+          GST_INFO ("%s", msg->str);
> >+          break;
> >+        default:
> >+          GST_ERROR ("%s", msg->str);
> >+      }
> >+
> >+      g_string_free (msg, TRUE);
> 
> We're now allocating, printf-ing and freeing a GString (and various helper
> allocs used inside g_string_printf) even if debug logging is completely
> disabled or not active, this seems suboptimal to me :)
> 

Oh right! Thx

> Is there any reason not to just duplicate the statement for GST_INFO and
> GST_ERROR ?

I was thinking that there could be more cases in the future, so bigger switch.

> 
> If you *really* want to avoid this you can also use GST_CAT_LEVEL_LOG() and
> specify the level there.

Thx for the suggestion I will try it to see how it looks.
Comment 21 Julien Isorce 2016-11-15 11:53:26 UTC
(In reply to Tim-Philipp Müller from comment #19)
> Comment on attachment 337175 [details] [review] [review]
> memory: log with GST_INFO instead GST_ERROR when subclass map failed.
> 
> Anything left to do here?

Sorry for the late reply I was off last week. Nothing left to be done. Since a few weeks I exactly know why we get mmap failure with EACCES (I did not know exactly the reason when I submitted the patch). I confirm this is not avoidable so still needed. (for the reason see https://bugzilla.gnome.org/show_bug.cgi?id=755072#c59 and https://bugzilla.gnome.org/show_bug.cgi?id=755072#c63)
Comment 22 Julien Isorce 2016-11-17 21:18:50 UTC
Created attachment 340188 [details] [review]
gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied

Add content in commit message. And use GST_CAT_LEVEL_LOG.
Comment 23 Julien Isorce 2016-11-21 21:28:13 UTC
Comment on attachment 337175 [details] [review]
memory: log with GST_INFO instead GST_ERROR when subclass map failed.

commit 47fd993d4d160f409301ccc108d943a38358fae1
Author: Julien Isorce <j.isorce@samsung.com>
Date:   Fri Oct 7 11:39:26 2016 +0100

    memory: log with GST_INFO instead GST_ERROR when subclass map failed.
    
    Add unit test to ensure that.
    
    It can be a normal execution path to do some map trials and there is
    no need to worry the user in that case.
    
    The application has to check the return value of gst_memory_map.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=765600
Comment 24 Julien Isorce 2016-11-21 21:28:47 UTC
Comment on attachment 340188 [details] [review]
gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied

commit b68d9bbe436d6d62b75572a563853d86783660e0
Author: Julien Isorce <j.isorce@samsung.com>
Date:   Fri Oct 7 15:08:37 2016 +0100

    gstfdmemory: log with GST_INFO instead of GST_ERROR on permission denied
    
    For example mmap can fail with EACCES if the the fd has been open
    with read only mode. And mapping the memory might be the only way
    to check that. So no need to print out an error.
    
    Ex: ioctl(dev, DRM_IOCTL_PRIME_HANDLE_TO_FD, flags & ~DRM_RDWR)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=765600