Created attachment 345525 [details] [review]
Allow GLMemory static caps

Comment on attachment 345525 [details] [review]
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.

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.

Created attachment 346061 [details] [review]
Support resizing; pass frame pointer to avoid calls to gst_video_encoder_get_frame in background threads; support gst_core_video buffers

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

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.

(In reply to Nicolas Dufresne (stormer) from comment #6)
> 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.

Review of attachment 346061 [details] [review]:

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

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);

(In reply to Nick Kallen from comment #9)

> The code does a lot of this:
> 
>   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.

Nick?

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/522.