After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 733087 - Add WebP Image encoder
Add WebP Image encoder
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-12 10:16 UTC by sreerenj
Modified: 2014-08-11 13:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add WebP Image encoder (16.42 KB, patch)
2014-07-12 10:16 UTC, sreerenj
needs-work Details | Review
Add WebP Image encoder (16.02 KB, patch)
2014-08-11 13:03 UTC, sreerenj
committed Details | Review

Description sreerenj 2014-07-12 10:16:16 UTC
Created attachment 280545 [details] [review]
Add WebP Image encoder

This is to add a new plugin element for WebP image encoding.
Comment 1 sreerenj 2014-08-08 23:14:06 UTC
ping?
Comment 2 Sebastian Dröge (slomo) 2014-08-11 08:10:32 UTC
Review of attachment 280545 [details] [review]:

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?
Comment 3 sreerenj 2014-08-11 12:37:39 UTC
(In reply to comment #2)
> Review of attachment 280545 [details] [review]:
> 
> 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

make sense
Comment 4 sreerenj 2014-08-11 12:39:10 UTC
(In reply to comment #2)
> Review of attachment 280545 [details] [review]:
> 
> > @@ +233,3 @@
> +  info = &enc->input_state->info;
> +
> +  if (!WebPPictureInit (&enc->webp_picture)) {
> 
> Isn't it necessary to deinit/free this at some point?
> 

please see the  WebPPictureFree() in handle_frame() implementation
Comment 5 sreerenj 2014-08-11 12:40:52 UTC
(In reply to comment #2)
> Review of attachment 280545 [details] [review]:
> 
> 
> @@ +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?
> 
Please see the "free (enc->webp_writer.mem)" in handle_frame() which is the only thing we needed.
Comment 6 sreerenj 2014-08-11 12:45:20 UTC
(In reply to comment #2)
> Review of attachment 280545 [details] [review]:

> 
> @@ +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 :)
> 

gst_buffer_new_and_alloc() is deprecated, will use 
gst_buffer_new_allocate() instead.. ;)
Comment 7 sreerenj 2014-08-11 12:46:05 UTC
(In reply to comment #2)
> Review of attachment 280545 [details] [review]:
> 

> @@ +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?
> 

NO
Comment 8 sreerenj 2014-08-11 12:49:11 UTC
(In reply to comment #2)
> Review of attachment 280545 [details] [review]:
> 
> Looks good, just some minor comments and questions

> @@ +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?

WebpConfig.method is "quality/speed trade-off (0=fast, 6=slower-better)" . The word speed is more appropriate for user. The word "method" doesn't make much sense from users perspective IMHO.
Comment 9 Sebastian Dröge (slomo) 2014-08-11 12:57:58 UTC
(In reply to comment #8)
> (In reply to comment #2)
> > Review of attachment 280545 [details] [review] [details]:
> > 
> > Looks good, just some minor comments and questions
> 
> > @@ +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?
> 
> WebpConfig.method is "quality/speed trade-off (0=fast, 6=slower-better)" . The
> word speed is more appropriate for user. The word "method" doesn't make much
> sense from users perspective IMHO.

Makes sense, just that we did the same for vp8enc and in the end it was more confusing for users because on the WebM wiki these things were called different :)
Comment 10 sreerenj 2014-08-11 13:03:16 UTC
Created attachment 283098 [details] [review]
Add WebP Image encoder
Comment 11 sreerenj 2014-08-11 13:08:17 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #2)
> > > Review of attachment 280545 [details] [review] [details] [details]:
> > > 
> > > Looks good, just some minor comments and questions
> > 
> > > @@ +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?
> > 
> > WebpConfig.method is "quality/speed trade-off (0=fast, 6=slower-better)" . The
> > word speed is more appropriate for user. The word "method" doesn't make much
> > sense from users perspective IMHO.
> 
> Makes sense, just that we did the same for vp8enc and in the end it was more
> confusing for users because on the WebM wiki these things were called different
> :)

Ah okay :)
cq-level for vp8enc?
Comment 12 sreerenj 2014-08-11 13:11:53 UTC
Do you want to change it to "method" ?
Personally I still prefer to keep "speed" :)
Comment 13 Sebastian Dröge (slomo) 2014-08-11 13:23:10 UTC
(In reply to comment #11)
> 
> Ah okay :)
> cq-level for vp8enc?

No, speed in 0.10 :)

I think it's ok, we can still change it as long as it's in -bad.
Comment 14 Sebastian Dröge (slomo) 2014-08-11 13:25:29 UTC
commit b9105792da50bea1f31ce9691a26300d5e30bb46
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Mon Aug 11 16:01:32 2014 +0300

    webenc: Add WebP image encoder
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733087