GNOME Bugzilla – Bug 733087
Add WebP Image encoder
Last modified: 2014-08-11 13:25:33 UTC
Created attachment 280545 [details] [review] Add WebP Image encoder This is to add a new plugin element for WebP image encoding.
ping?
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?
(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
(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
(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.
(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.. ;)
(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
(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.
(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 :)
Created attachment 283098 [details] [review] Add WebP Image encoder
(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?
Do you want to change it to "method" ? Personally I still prefer to keep "speed" :)
(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.
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