Comment on attachment 301113 [details] [review]
make gst_base_text_overlay_push_frame perform deep copy when memory of a buffer is read only

This doesn't look right, and shouldn't be needed.

When gst_buffer_map() is asked to map for writing and the memory is not writable for some reason (e.g. because it has been flagged as readonly, or is shared between multiple buffers), a copy will be made automatically before mapping.

/**
 * gst_buffer_map:
 *
 * @flags describe the desired access of the memory. When @flags is
 * #GST_MAP_WRITE, @buffer should be writable (as returned from
 * gst_buffer_is_writable()).
 *
 * When @buffer is writable but the memory isn't, a writable copy will
 * automatically be created and returned. The readonly copy of the
 * buffer memory will then also be replaced with this writable copy.

It sounds to me like maybe something your decoder does is not quite right?

If I set the flag of memory in buffer as READONLY.(via setting GST_MEMORY_FLAG_READONLY to its pool allocator to make the allocated memory is read only). then

video_frame = gst_buffer_make_writable (video_frame);
gst_video_frame_map(&frame, &overlay->info, video_frame,GST_MAP_READWRITE);

I got failed with following error:

ERROR                default video-frame.c:141:gst_video_frame_map_id: failed to map video frame plane 0

Did i missed something? or is there any interface need to be implemented? 

the use case is:
we use a hardware decoder, so the buffer was allocated by this hardware decoder.
but for some format, say H264, this decoder need reference the outputed frame. so the output frame can't be modified. but in subtitle case, we need blending text on to the output frame, in this case, we need copy the original output frame, then blend and output the copied buffer.

If the frame can't be modified, the memory should automatically be copied when mapping with MAP_READWRITE.

I'm not sure what's happening here, but it sounds like a bug elsewhere.

Perhaps you could you make a minimal unit test that demonstrates the issue?

Review of attachment 301113 [details] [review]:

This patch should not exist imho. A memory will only be copied by gst_buffer_make_writable() if the NO_SHARE flag is found on GstMemory. If a copy at this point is not wanted, then properly track your memory lifetime (common mistake in custom pool implementation) and drop the NO_SHARE flag.

Then, gst_video_frame_map() will use standard map API which have exactly the wanted behaviour (as describe in all the messy comments). Basically, if the memory is locked WRITE + EXCLUSIVE, or has the READONLY flags, it will be copied, and replaced inside the writable buffer (if not writable buffer, there is a warning, and you'll have issues, as the buffer push back would not be modified). This patch is exactly how you "Upstream Status" mask indicate. I left few comments to be considered in further submissions.

::: ext/pango/gstbasetextoverlay.c
@@ +748,3 @@
       f = gst_caps_features_new
           (GST_CAPS_FEATURE_META_GST_VIDEO_OVERLAY_COMPOSITION, NULL);
+      gst_caps_set_features(overlay_caps, 0, f);

This change is unrelated, and is out of sync with master (where this bug does not exist).

@@ +1906,3 @@
+  GstBuffer *orig = video_frame;
+
+  while (--m>=0) {

Avoid these obscure while() loops, use for loops instead.

@@ +1917,3 @@
+
+  if (mem_rdonly) {
+    // since gst_buffer_make_writable just lookup the refcount to determine if

never use C++ style comments

(In reply to Mingke Wang from comment #2)
> ERROR                default video-frame.c:141:gst_video_frame_map_id:
> failed to map video frame plane 0
> 
> Did i missed something? or is there any interface need to be implemented? 

This can fail in two ways, you can get this information if you enable GST_DEBUG="2,videometa:7".

a) "plane %u, no memory at offset %" G_GSIZE_FORMAT,

That would mean no GstMemory could be found at the expected offset for this plane. This could indicate that upstream element (you didn't mention which one) didn't set the offset[] array of the videometa properly, or that the memory dimensions are not set properly.


b) "cannot map memory range %u-%u", idx, length);

That would indicate that the memory could not be map, even for read + copy. This would indicate that the memory is already mapped WRITE only by an upstream element.

In both cases it's not textoverlay's fault. I would like you to provide information about the entire pipeline you are running, as the problem is more likely in the element the provides these buffer/memory.

Thanks guys.

I finial got time to review my previous codes again and found that gst_video_frame_map fail is my allocator implementation problem. I copied the memory flag from source memory into destination memory in mem_copy() implementation which caused the copied memory is also read only.

but got new problem:
even gst_video_frame_map() can make a writable memory. but it will replace the original memory in the buffer with the writable memory. that is not acceptable by our decoder element. we use hardware decoder which require registering memories before starting decoding. so the memory in a buffer which pushed by our decoder is not allowed to be replaced. 

what I expected here is that:

if (buffer_is_writable && memory_is_read_only && buffer_memory_is_not_replaceable) {
   create_new_buffer_with_new_copied_writable_memory;
   gst_video_frame_map(new_buffer);
}

Any good idea for my case without changing gst_base_text_overlay_push_frame?

If the GstBuffer is read-only, it will replace the GstBuffer first. If it's writable, it will simply replace the memory. That's why your decoder need to track it's memory, instead of it's buffer. A buffer pool is just a cache to try and avoid allocation GstBuffer object over and over.

Note that it would be nice if the text overlay does not do in-place when mapping the frame would lead to a copy. This way we could allocate from a downstream pool, but that's obviously more work.

(In reply to Nicolas Dufresne (stormer) from comment #7)
> If the GstBuffer is read-only, it will replace the GstBuffer first. If it's
> writable, it will simply replace the memory. That's why your decoder need to
> track it's memory, instead of it's buffer. A buffer pool is just a cache to
> try and avoid allocation GstBuffer object over and over.

The hardware decoder need physical continuous memory. actually, the physical memories are allocated by the hardware decoder and registered to it at the initialization. then we implemented a alocator to wrap those memories to gst buffer. so the GstBuffer allocation only happens at the beginning. and once memory registered, it can't be changed on the fly unless stop and initial again.

(In reply to Nicolas Dufresne (stormer) from comment #8)
> Note that it would be nice if the text overlay does not do in-place when
> mapping the frame would lead to a copy. This way we could allocate from a
> downstream pool, but that's obviously more work.

I think one possible change could be  gst_buffer_make_writable().
now it will copy a new GstBuffer with new writable memory only when the GstBuffer is read-only. 
We could add a case :
copy a new GstBuffer when (GstBuffer is writable && memory is read-only && memory can't be replaced)
that require add a new flag definition to GstBuffer flag like GST_BUFFER_FLAG_MEMORY_NOT_REPLACEABLE

Well, at that point, you're better to add the NO_SHARE flag to your memory. It will do what you want, generally speaking it's not the best approach.

Anything to add, or can be mark this bug as not our bug ?

(In reply to Nicolas Dufresne (stormer) from comment #12)
> Anything to add, or can be mark this bug as not our bug ?

Yes, it is really hardware specific. BTW, how to mark this bug not general base bug?