[Bug 627459] theoraenc should provide option for TH_ENCCTL_SET_DUP_COUNT

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=627459
  GStreamer | gst-plugins-base | 0.10.x

--- Comment #49 from Vincent Penquerc'h <vincent.penquerch at collabora.co.uk> 2011-12-19 15:09:04 UTC ---
Review of attachment 191183:
 --> (https://bugzilla.gnome.org/review?bug=627459&attachment=191183)

Update common, it should not be included in the patch.
Also please try to fix all the typos.

::: ext/theora/gsttheoraenc.c
@@ +424,3 @@
+  g_object_class_install_property (gobject_class, PROP_DUP_ON_GAP,
+      g_param_spec_boolean ("dup-on-gap", "Create DUP frame on GAP flag",
+          "Allow codec to handle frames with GAP flag as duplicates of next
frame. "

Of previous frame, right ?

@@ +573,3 @@
   enc->timestamp_offset = 0;

+  theora_timefifo_free (enc);

Will leave the queue pointer dangling. NULL it in that function.

@@ +940,3 @@
       gst_segment_init (&enc->segment, GST_FORMAT_UNDEFINED);
       res = gst_pad_push_event (enc->srcpad, event);
+      theora_timefifo_free (enc);

Dangling too.

@@ +1174,3 @@
+
+static void
+theora_timefifo_in (GstTheoraEnc * enc, GstClockTime * timestamp)

Either pass a GstClockTime, or make the pointer const.

@@ +1239,3 @@
+    if (g_queue_get_length (enc->t_queue))
+      g_queue_foreach (enc->t_queue, (GFunc) theora_free_gstclocktime, NULL);
+    g_queue_free (enc->t_queue);

enc->t_queue = NULL; here to avoid dangling pointers above.

@@ +1318,3 @@
+            t_queue_length -= 2;
+            res = th_encode_ctl (enc->encoder, TH_ENCCTL_SET_DUP_COUNT,
+                &t_queue_length, sizeof (t_queue_length));

This is setting dup count for every gap frame, would it not be better to only
set it once we know, before sending the actual frame for encoding ?

@@ +1331,3 @@
+      if (t_queue_length > 1) {
+        res = th_encode_ctl (enc->encoder, TH_ENCCTL_SET_DUP_COUNT,
+            &t_queue_length, sizeof (t_queue_length));

I think you're setting one too much here. For N frames, you encode the frame
and N-1 dups.

::: ext/theora/gsttheoraenc.h
@@ +143,3 @@
 GType gst_theora_enc_get_type (void);
+static GstFlowReturn
+theora_enc_encode_and_push (GstTheoraEnc * enc, ogg_packet op,

Why is this here ? It should not be used anywhere else than gsttheoraenc.c.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.