[Bug 743155] applemedia: new AVSampleBufferLayerSink

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #294835|none                        |needs-work
             status|                            |

--- Comment #2 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2015-01-19 09:06:11 UTC ---
Review of attachment 294835:
 --> (https://bugzilla.gnome.org/review?bug=743155&attachment=294835)

::: sys/applemedia/avsamplevideosink.m
@@ +22,3 @@
+ * SECTION:element-av_sink
+ *
+ * av_sink renders video frames to a drawable on a local or remote

Still OpenGL stuff in the docs :)

@@ +119,3 @@
+    GST_PAD_SINK,
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE ("{ RGB, BGR, ARGB, BGRA, ABGR, RGBA
}"))

Support for I420 and/or NV12 would be nice :) It should support that in theory,
at least for the VideoToolbox encoder we can wrap I420 and NV12 in
CVPixelBuffers.

@@ +125,3 @@
+{
+  ARG_0,
+  PROP_FORCE_ASPECT_RATIO,

ARG_0 and PROP_STUFF (inconsistent)

@@ +243,3 @@
+
+static gboolean
+gst_av_sample_video_sink_query (GstBaseSink * bsink, GstQuery * query)

Can go away :)

@@ +261,3 @@
+  GstAVSampleVideoSink *av_sink = GST_AV_SAMPLE_VIDEO_SINK (bsink);
+
+  av_sink->layer = [[AVSampleBufferDisplayLayer alloc] init];

Is this safe to be called from any thread? Maybe needs to use dispatch_sync()
with the main queue here if not already on the main thread

@@ +280,3 @@
+
+static GstStateChangeReturn
+gst_av_sample_video_sink_change_state (GstElement * element, GstStateChange
transition)

Can go away too :)

@@ +425,3 @@
+  }
+
+  ret = gst_caps_simplify (ret);

You could also use the GstValue API to directly create the array of strings
instead of merging caps like this. It would also be less string-programming
(gst_caps_from_string() above)

@@ +481,3 @@
+    return FALSE;
+
+  GST_TRACE ("PAR: %u/%u DAR:%u/%u", par_n, par_d, display_par_n,

Use GST_TRACE_OBJECT() and also the _OBJECT() variants for all the other things

@@ +493,3 @@
+    GST_DEBUG ("keeping video width");
+    GST_VIDEO_SINK_WIDTH (av_sink) = width;
+    GST_VIDEO_SINK_HEIGHT (av_sink) = (guint)

These values should probably be set as size of the layer or something like
that, so that CA knows the "original" size and the aspect ratio at least

@@ +507,3 @@
+  av_sink->info = vinfo;
+
+  newpool = gst_video_buffer_pool_new ();

At a later point it might make sense to create a pool around a
CVPixelBufferPool

@@ +582,3 @@
+  buf_attrs = CFDictionaryCreate (NULL, NULL, NULL, 0,
&kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks);
+
+  if (kCVReturnSuccess != CVPixelBufferCreateWithBytes (NULL,
GST_VIDEO_INFO_WIDTH (&av_sink->info),

There already is API to wrap a CVPixelBuffer around a GstBuffer inside
corevideo.m IIRC

@@ +616,3 @@
+  sample_time.duration = CMTimeMake (GST_BUFFER_DURATION (buf), GST_SECOND);
+  sample_time.presentationTimeStamp = CMTimeMake (GST_BUFFER_PTS (buf),
GST_SECOND);
+  sample_time.decodeTimeStamp = kCMTimeInvalid;

This probably causes problems if a special segment is configured, or the sink
is added later to the pipeline. See all the work that was necessary in
decklinkvideosink to make that work :) Also needs clock slaving with the
CoreMedia clock probably.

I think initially this should just use
kCMSampleAttachmentKey_DisplayImmediately, we already synced to the pipeline
clock for the timestamp at this point

@@ +626,3 @@
+  [(AVSampleBufferDisplayLayer *)av_sink->layer
enqueueSampleBuffer:sample_buf];
+
+  gst_video_frame_unmap (&v_frame);

The unmapping should happen in _unref_buffer() only, otherwise the CALayer
might use already unmapped memory

::: tests/examples/Makefile.am
@@ +27,3 @@
+if HAVE_AVFOUNDATION
+if HAVE_IOS
+AVSAMPLE_DIR=

It's not available on iOS?

::: tests/examples/avsamplesink/main.m
@@ +69,3 @@
+  main_loop = g_main_loop_new (NULL, FALSE);
+
+  [NSApplication sharedApplication];

This is generally not a good idea, using a GMainLoop for creating some Cocoa UI
application. It currently only works because of our custom GLib patches :)
It would be better to use the normal NSRunLoop instead

-- 
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.