[Bug 733087] Add WebP Image encoder

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=733087
  GStreamer | gst-plugins-bad | unspecified

Sebastian Dröge (slomo) <slomo> changed:

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

--- Comment #2 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-08-11 08:10:32 UTC ---
Review of attachment 280545:
 --> (https://bugzilla.gnome.org/review?bug=733087&attachment=280545)

Looks good, just some minor comments and questions

::: ext/webp/gstwebpenc.c
@@ +220,3 @@
+
+  if (enc->input_state)
+    gst_video_codec_state_unref (enc->input_state);

You should probably get rid of the state in stop() already

@@ +233,3 @@
+  info = &enc->input_state->info;
+
+  if (!WebPPictureInit (&enc->webp_picture)) {

Isn't it necessary to deinit/free this at some point?

@@ +245,3 @@
+  enc->webp_picture.height = GST_VIDEO_INFO_HEIGHT (info);
+
+  WebPMemoryWriterInit (&enc->webp_writer);

Isn't it necessary to deinit/free this at some point?

@@ +316,3 @@
+    gst_memory_unmap (mem, &mapinfo);
+    free (enc->webp_writer.mem);
+    gst_buffer_append_memory (out_buffer, mem);

Why not just gst_buffer_new_and_alloc(size) and gst_buffer_fill()? Much less
lines :)

@@ +392,3 @@
+  GstWebpEnc *enc = (GstWebpEnc *) benc;
+
+  if (!WebPConfigPreset (&enc->webp_config, enc->preset, enc->quality)) {

Isn't it necessary to deinit/free this at some point?

@@ +398,3 @@
+
+  enc->webp_config.lossless = enc->lossless;
+  enc->webp_config.method = enc->speed;

Why do you call it speed, while the webp API calls it method?

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