Discussion:
[Bug 778496] New: VTENC should support GLMemory
(too old to reply)
"GStreamer" (GNOME Bugzilla)
2017-02-11 15:09:10 UTC
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
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
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
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
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
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
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
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.
"GStreamer" (GNOME Bugzilla)
2017-02-26 10:23:47 UTC
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

--- Comment #7 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Nicolas Dufresne (stormer) from comment #6)
Post by "GStreamer" (GNOME Bugzilla)
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.
That's not true, to be able to remove the lock there would have to be quite
some refactoring in the base class.
--
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-26 10:24:33 UTC
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

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

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345525|needs-work |accepted-commit_now
status| |
--
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-26 10:31:27 UTC
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

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

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

--- Comment #8 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 346061:
--> (https://bugzilla.gnome.org/review?bug=778496&attachment=346061)

This looks like it should be split into 3 different patches. You have 3
separate functional changes in here, even your commit message reads like that
;)

::: sys/applemedia/vtenc.c
@@ +652,3 @@
+ GstStructure *s;
+ gboolean peer_has_resolution;
+ gint peer_width, peer_height;

Variable declarations in C code should be at the beginning of the block

@@ +661,3 @@
+ if (peer_has_resolution) {
+ self->negotiated_width = peer_width;
+ self->negotiated_height = peer_height;

This might be better to do in negotiate()

@@ +745,3 @@
self->input_state);
+ state->info.width = self->negotiated_width;
+ state->info.height = self->negotiated_height;

You need to correct the pixel-aspect-ratio here AFAIU. If the encoder was
rescaling, it might've kept the display-aspect-ratio but you need to also
update the pixel-aspect-ratio in the caps accordingly then

@@ +823,3 @@
+ g_value_init (&value, GST_TYPE_INT_RANGE);
+ gst_value_set_int_range (&value, 1, G_MAXINT);
+ gst_structure_set_value (caps_s, "width", &value);

Ideally g_value_unset(&value) after you're done with it. Doesn't make a
difference here (int ranges don't occupy heap memory) but is cleaner

@@ +864,3 @@
+
+ if (new_filter != NULL)
+ gst_caps_unref (new_filter);

You need to intersect with the original filter before returning, otherwise you
might return a wider range of resolutions than upstream wanted

@@ +1322,3 @@
GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
vt_status = VTCompressionSessionEncodeFrame (self->session,
+ pbuf, ts, duration, frame_props, frame, NULL);

Reason for going via the frame number here is that it can prevent access to
already freed memory (or memory leaks if you pass a new reference here). When
getting it again later by integer, we can handle properly if the frame was
freed already (can probably happen during shutdown). (Or if you pass another
reference, we can ensure that the encoder is never keeping the reference
forever but we can free the frame when we want).
--
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-26 18:35:26 UTC
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com
--
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-03-04 09:48:42 UTC
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

--- Comment #9 from Nick Kallen <***@gmail.com> ---
OK I'm going to deal with all these changes this week.
Reason for going via the frame number here is that it can prevent access to already
freed memory (or memory leaks if you pass a new reference here). When getting it
again later by integer, we can handle properly if the frame was freed already (can
probably happen during shutdown). (Or if you pass another reference, we can ensure
that the encoder is never keeping the reference forever but we can free the frame
when we want).
OK I'll revert this change. It seems unfortunate because when getting by
integer a lock is acquired, and you have all this deadlock avoidance code in
vtenc which as I discovered was missing several cases. The issue is you can
call apples encoder, and it will (sometimes) block, but it will actually do the
work in another thread. And if you call it with a lock which it will later need
in the other thread, you get a deadlock.

The code does a lot of this:

GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
VTCompressionSessionCompleteFrames (self->session,
kCMTimePositiveInfinity);
GST_VIDEO_ENCODER_STREAM_LOCK (self);
--
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-03-04 09:55:56 UTC
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778496

--- Comment #10 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Nick Kallen from comment #9)
Post by "GStreamer" (GNOME Bugzilla)
GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
VTCompressionSessionCompleteFrames (self->session,
kCMTimePositiveInfinity);
GST_VIDEO_ENCODER_STREAM_LOCK (self);
Taking the encoder's stream lock is required from the other threads to do
anything with the GstVideoEncoder. If you don't do anything else with it and
you can guarantee that the GstVideoCodecFrame* you pass around directly is a)
always ending up in your callback (so that you can unref it) and b) you always
get it once and only the ones you provided (e.g. not some random other pointer
sometimes), then you can also pass the frame directly. It's just more
requirements on the VT API which you have to check to ensure everything is
memory-safe.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
Loading...