[Bug 747495] gst_base_text_overlay_push_frame() doesn't make memory copy when video buffer's memory is ready only

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=747495

--- Comment #4 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
Review of attachment 301113:
 --> (https://bugzilla.gnome.org/review?bug=747495&attachment=301113)

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

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.