[Bug 778496] vtenc: should support GLMemory

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778496

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

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

--- Comment #8 from Sebastian Dröge (slomo) <slomo at 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.