Discussion:
[Bug 778496] New: VTENC should support GLMemory
Add Reply
"GStreamer" (GNOME Bugzilla)
2017-02-11 15:09:10 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

Bug ID: 778496
Summary: VTENC should support GLMemory
Classification: Platform
Product: GStreamer
Version: 1.11.1
OS: Mac OS
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-bad
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@gmail.com
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

Currently vtenc caps disallow GLMemory. However, having tested this on iOS and
reviewed the code, it actually works using GLMemory. Since the underlying pixel
buffer is always a core media buffer,

pbuf = gst_core_media_buffer_get_pixel_buffer (frame->input_buffer);

this statement is valid whether the input_buffer's pixel buffer is GL buffer or
not.

Therefore, just changing the static caps works.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-11 15:14:30 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

--- Comment #1 from Nick Kallen <***@gmail.com> ---
Created attachment 345525
--> https://bugzilla.gnome.org/attachment.cgi?id=345525&action=edit
Allow GLMemory static caps
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-11 16:09:29 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

Tim-Philipp Müller <***@zen.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@zen.co.uk
Summary|VTENC should support |vtenc: should support
|GLMemory |GLMemory
Severity|normal |enhancement
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 10:22:01 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net
Attachment #345525|none |needs-work
status| |

--- Comment #2 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Comment on attachment 345525
--> https://bugzilla.gnome.org/attachment.cgi?id=345525
Allow GLMemory static caps

You're removing support for caps *without* GL feature here. Also are you sure
about the formats? I would've expected BGRA or a variant of that for GL.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-15 11:23:10 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

--- Comment #3 from Nick Kallen <***@gmail.com> ---
Correct me if I'm wrong, but doesn't

"; " GST_VIDEO_CAPS_MAKE ("{ NV12, I420 }")

includes the old formats?

And yeah I'm 98% about NV12 and I420 -- these are the formats specific to the
hardware encoding and decoding on the iphone.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-17 12:22:32 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

--- Comment #4 from Nick Kallen <***@gmail.com> ---
Created attachment 346061
--> https://bugzilla.gnome.org/attachment.cgi?id=346061&action=edit
Support resizing; pass frame pointer to avoid calls to
gst_video_encoder_get_frame in background threads; support gst_core_video
buffers
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-17 12:29:03 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

--- Comment #5 from Nick Kallen <***@gmail.com> ---
This latest patch is non-trivial and although it works correctly I wanted to
get feedback on implementation details.

Firstly, the old code was extremely deadlock-prone, look at code like this:

/* We need to unlock the stream lock here because
* it can wait for gst_vtenc_enqueue_buffer() to
* handle a buffer... which will take the stream
* lock from another thread and then deadlock */
GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
vt_status =
VTCompressionSessionCompleteFrames (self->session,
kCMTimePositiveInfinity);

The main problem is the various VTC* methods may or may not block, but they
usually execute code asynchronously. And the callback in this case always
called gst_video_encoder_get_frame, which required the video encoder stream
lock.

I haven't removed these UNLOCKs in this commit yet, just to be careful, but I'm
pretty sure this removes all possibility of deadlock. But it relies on passing
frame pointers between threads -- I wanted to double check that's safe. Seems
like it should be, but it's surprising the old code didn't do that in the first
place.

--

The resizing-related __unfix_resolution code is elaborate but turned out to be
necessary to get caps to agree. The pipelines I have in mind are:

uridecodebin ! vtenc ! video/x-h264,width=x,height=y
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-17 14:49:04 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@ndufresne.ca

--- Comment #6 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Just a note that the encoder stream_lock is kind of historical. It's redundant
with the pads stream locks, and does not provide the same level of flexibility.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
Loading...