Created attachment 312005 [details] [review]
vpx: created common base class GstVPXEnc for encoders

created common base class for vpx encoders

Created attachment 312006 [details] [review]
vpx: created common base class GstVPXDec for decoders

created common base class for decoders

Review of attachment 312006 [details] [review]:

Some comment, will need to be rebased first. Let me know if you have time, otherwise I'll take care.

::: ext/vpx/gstvp8dec.c
@@ +129,3 @@
 {
+  gst_tag_list_add (list, GST_TAG_MERGE_REPLACE,
+      GST_TAG_VIDEO_CODEC, "VP8 video", NULL);

Could also be a static const char on the class structure.

@@ +136,2 @@
 {
+  return &vpx_codec_vp8_dx_algo;

We should use class attribute (members on the class structure) for static stuff.

@@ +143,3 @@
 {
+  /*nothing to do here as stream info set by call peek_stream_info is correct for vp8 */
+  return;

Can't we just not implement virtual functions we don't need ?

@@ +157,2 @@
   gst_video_decoder_negotiate (GST_VIDEO_DECODER (dec));
+  vpxclass->send_tags (dec);

I have the impression there is no reason to have to call this in the subclass.

@@ +160,3 @@
 
+static gboolean
+gst_vp8_dec_get_valid_format (GstVPXDec * dec, vpx_image_t * img,

I'd try to find a better name for this one.

@@ +174,3 @@
+static void
+gst_vp8_dec_handle_resolution_change (GstVPXDec * dec, vpx_image_t * img,
+    GstVideoFormat fmt)

I would not find (except for the hardcoded I420) any reason why this isn't generic. Any idea ?

::: ext/vpx/gstvp9dec.c
@@ +152,3 @@
+   * As this element supports multiple formats, format will be negotiated dynamically while handling frame
+   */
+  return;

It should be static, or at least no have to implement.

@@ +203,2 @@
+    if (send_tags)
+      vpxclass->send_tags (dec);

It's suspicious that this differ.

(In reply to Nicolas Dufresne (stormer) from comment #3)

> ::: ext/vpx/gstvp8dec.c
> @@ +129,3 @@
>  {
> +  gst_tag_list_add (list, GST_TAG_MERGE_REPLACE,
> +      GST_TAG_VIDEO_CODEC, "VP8 video", NULL);
> 
> Could also be a static const char on the class structure.

Note that there is also API for that in pbutils to set the codec tag based on the caps. Then we only need to hardcode this string in a single place.

thanks for the review. I will work on it and will submit the updated patch for the decoder

Created attachment 317401 [details] [review]
vpx: created common base class GstVPXDec for decoders

please review updated patch to create GstVPXDec base class for vpx decoders.
Patch is updated as per the review comments and recent new changes committed to vp8decoder and vp9decoder for the allocation.

Review of attachment 317401 [details] [review]:

Looks good to me. There very few weirdness and arguably they already exist in master. One is the streaminfo bug, I suppose it's a libvpx bug, the other is the tags, but I'm open to merge this great effort anyway, it will greatly reduce the maintenance work needed.

A small, but really annoying issue, have_video_meta is never set anymore.

Ah, the add_video_meta was missing in the vp8 case and you though this was normal. Most differences between these two are not normal.

Comment on attachment 317401 [details] [review]
vpx: created common base class GstVPXDec for decoders

commit 90dcc3921a62d766c7e3ddb0fe21884af6e73910
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Tue Dec 15 12:57:53 2015 -0500

    vpxdec: Remove unneeded add video_meta
    
    This also remove copies for VP8, which was not correctly in place
    in previous related patch.

commit 75fb614c1e9dea78389533f3009b0259a2b9bf8e
Author: Prashant Gotarne <ps.gotarne@samsung.com>
Date:   Tue Dec 15 09:49:24 2015 +0530

    vpx: created common base class GstVPXdec for vpx decoders
    
    Base class for the vp8dec and vp9dec.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=755510

I'll take care of the next one after lunch.

Thanks for this work. We'll keep watching for any regression, but my basic testing seems to indicates it works properly.

distcheck is not happy:

../../../ext/vpx/gstvpxdec.c:29:23: fatal error: gstvpxdec.h: No such file or directory
 #include "gstvpxdec.h"
                       ^

 https://ci.gstreamer.net/job/GStreamer-master-distcheck/2824/