[Bug 731204] androidmedia: Implement zerocopy rendering

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=731204
  GStreamer | gst-plugins-bad | git

Nicolas Dufresne <nicolas.dufresne> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #278417|none                        |reviewed
             status|                            |

--- Comment #18 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2014-06-18 14:17:12 UTC ---
Review of attachment 278417:
 --> (https://bugzilla.gnome.org/review?bug=731204&attachment=278417)

Partial review, if we can split all the style fixes and merge this already, we
could shrink a bit this patch.

::: sys/androidmedia/gstamc.c
@@ +3485,3 @@
               profile =
+                  gst_amc_mpeg4_profile_to_string (type->profile_levels[j].
+                  profile);

These look like style fixex, can you split it up ? These can be merge already,
would remove some noise in your branch.

::: sys/androidmedia/gstamcaudiodec.c
@@ +490,3 @@
       buffer_info.flags);

+  is_eos = !!(buffer_info.flags & BUFFER_FLAG_END_OF_STREAM);

Style again.

::: sys/androidmedia/gstamcvideodec.c
@@ +528,3 @@
+  if (self->use_surface) {
+    gst_format = GST_VIDEO_FORMAT_RGBA;
+  }

No need for {}

@@ +714,3 @@
       gst_util_uint64_scale (buffer_info.presentation_time_us, GST_USECOND,
1));

+  is_eos = !!(buffer_info.flags & BUFFER_FLAG_END_OF_STREAM);

Style.

@@ +1088,3 @@
   }

+  /* FIXME, TODO: negotiate better ! */

shouldn't this be in negotiate() virtual method to better align with base class
negotiation code ? Also, define better, so we can give you some input if
needed.

@@ +1128,3 @@
+      gst_caps_unref (gl_texture_upload_caps);
+
+      GST_LOG ("Hardware decoding: %s", self->use_surface ? "enabled" :
"disabled");

That seems ambiguous trace, hardware decoding is possible even if not doing
direct rendering.

::: sys/androidmedia/gstamcvideomemory.c
@@ +293,3 @@
+  GstVideoGLTextureUploadMeta *meta = NULL;
+  GstVideoGLTextureType texture_types[] =
+      { GST_VIDEO_GL_TEXTURE_TYPE_OES, 0, 0, 0 };

Is it always a single texture/surface ? Could we consider the other way around,
which is to request _2D/RGBA texture, and do the conversion here, in our
upload() method ? I think it would require a way to query downstream GL
context, that minimally mean have access to the decoder src pad when upload is
called (see user data in the upload meta). Would that be possible ?

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.