GNOME Bugzilla – Bug 765600
gst_memory_map: delegate logging traces to subclass on failure
Last modified: 2016-11-21 21:29:44 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.
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
Ok I'll have a go this week I think.
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.
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
(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.
I don't really know, it's all a bit annoying really to just do this for not doing a GST_ERROR() :)
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.
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).
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)
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.
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 ?
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 ?
Created attachment 330237 [details] [review] memory: remove GST_ERROR on map failure
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.
(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.
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.
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 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 on attachment 337175 [details] [review] memory: log with GST_INFO instead GST_ERROR when subclass map failed. Anything left to do here?
(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.
(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)
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 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 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