GNOME Bugzilla – Bug 743155
applemedia: new AVSampleBufferLayerSink
Last modified: 2015-02-24 08:16:23 UTC
A new sink that provides a CALayer that can be placed inside a Core Animation render tree.
Created attachment 294835 [details] [review] applemedia: new AVSampleBufferLayerSink
Review of attachment 294835 [details] [review]: ::: 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
Created attachment 295070 [details] [review] applemedia: new AVSampleBufferLayerSink Anything not replied to below has been addressed. (In reply to comment #2) > Review of attachment 294835 [details] [review]: > @@ +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. No matter what I try, I cannot seem to get CVPixelBufferCreateWithPlanarBytes to work with NV12 so it has been commented out. There's still YUY2 and UYVY support though. > @@ +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 I have not looked extensively at the aspect ratio and how CA handle positioning and sizing but it seems that it works on a similar concept to the wayland model where sublayers are defined relative to their parent layer and so maybe a similar solution as waylandsink with its window inside a window for aspect ratio handling needs to be implemented here. > @@ +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 No, there's only functions to create GstBuffers from CVPixelBuffer's not create CVPixelBuffer's from a GstBuffer (unless you count a potential copy case). > ::: tests/examples/Makefile.am > @@ +27,3 @@ > +if HAVE_AVFOUNDATION > +if HAVE_IOS > +AVSAMPLE_DIR= > > It's not available on iOS? The example is not available on iOS due to NSWindow usage. The element should work on iOS though.
(In reply to comment #3) > Created an attachment (id=295070) [details] [review] > applemedia: new AVSampleBufferLayerSink > > Anything not replied to below has been addressed. > > (In reply to comment #2) > > Review of attachment 294835 [details] [review] [details]: > > @@ +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. > > No matter what I try, I cannot seem to get CVPixelBufferCreateWithPlanarBytes > to work with NV12 so it has been commented out. There's still YUY2 and UYVY > support though. There is code for vtenc to put I420 and NV12 into pixel buffers. But maybe that only works for VideoToolbox and not AVSampleBufferLayer. > > @@ +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 > > I have not looked extensively at the aspect ratio and how CA handle positioning > and sizing but it seems that it works on a similar concept to the wayland model > where sublayers are defined relative to their parent layer and so maybe a > similar solution as waylandsink with its window inside a window for aspect > ratio handling needs to be implemented here. Needs some research then, but this is a necessary feature IMHO :) CALayer has a contentsRect property though, and frame and bounds. Also the preferredFrameSize method. Maybe modifying one of these is already enough, or are they expected to be modified by the application? For NSView IIRC the frame is what we modify. > > @@ +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 > > No, there's only functions to create GstBuffers from CVPixelBuffer's not create > CVPixelBuffer's from a GstBuffer (unless you count a potential copy case). Yeah, it's in vtenc.c in gst_vtenc_encode_frame() in the #ifndef HAVE_IOS case (in the IOS case we copy).
Created attachment 295515 [details] [review] applemedia: new AVSampleBufferLayerSink Updated patch that chooses the aspect ratio settings for the layer based on the force-aspect-ratio property and correctly takes I420 input. NV12 is still broken though and is disabled.
Created attachment 296407 [details] [review] applemedia: new AVSampleBufferLayerSink Adds some minor memleak fixes on top of the previous.
Created attachment 296993 [details] [review] applemedia: new AVSampleBufferLayerSink g_object_notify the layer instantiation
commit 021bfa99a116fe4cbb9758cf8866e3dc75a296c8 Author: Matthew Waters <matthew@centricular.com> Date: Thu Jan 29 17:41:19 2015 +1100 new caopengllayersink element renders gstreamer gl scene/video frames to a caopengllayer retreivable from the "layer" property.