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 755510 - vpx: create base class for vpx encoders and decoders
vpx: create base class for vpx encoders and decoders
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-24 07:54 UTC by Prashant Gotarne
Modified: 2015-12-15 19:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vpx: created common base class GstVPXEnc for encoders (224.93 KB, patch)
2015-09-24 07:56 UTC, Prashant Gotarne
committed Details | Review
vpx: created common base class GstVPXDec for decoders (66.92 KB, patch)
2015-09-24 07:56 UTC, Prashant Gotarne
none Details | Review
vpx: created common base class GstVPXDec for decoders (80.22 KB, patch)
2015-12-15 04:19 UTC, Prashant Gotarne
committed Details | Review

Description Prashant Gotarne 2015-09-24 07:54:26 UTC
VP8 and VP9 encoders and decoders have almost 80% code common.
Create a base class for the VPX encoders and decoders to avoid duplication of code.
Comment 1 Prashant Gotarne 2015-09-24 07:56:01 UTC
Created attachment 312005 [details] [review]
vpx: created common base class GstVPXEnc for encoders

created common base class for vpx encoders
Comment 2 Prashant Gotarne 2015-09-24 07:56:53 UTC
Created attachment 312006 [details] [review]
vpx: created common base class GstVPXDec for decoders

created common base class for decoders
Comment 3 Nicolas Dufresne (ndufresne) 2015-12-04 20:09:23 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2015-12-05 08:32:23 UTC
(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.
Comment 5 Prashant Gotarne 2015-12-11 08:39:01 UTC
thanks for the review. I will work on it and will submit the updated patch for the decoder
Comment 6 Prashant Gotarne 2015-12-15 04:19:27 UTC
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.
Comment 7 Nicolas Dufresne (ndufresne) 2015-12-15 17:04:29 UTC
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.
Comment 8 Nicolas Dufresne (ndufresne) 2015-12-15 17:25:39 UTC
A small, but really annoying issue, have_video_meta is never set anymore.
Comment 9 Nicolas Dufresne (ndufresne) 2015-12-15 17:47:53 UTC
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 10 Nicolas Dufresne (ndufresne) 2015-12-15 18:00:52 UTC
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
Comment 11 Nicolas Dufresne (ndufresne) 2015-12-15 18:01:09 UTC
I'll take care of the next one after lunch.
Comment 12 Nicolas Dufresne (ndufresne) 2015-12-15 19:08:30 UTC
Thanks for this work. We'll keep watching for any regression, but my basic testing seems to indicates it works properly.
Comment 13 Nicolas Dufresne (ndufresne) 2015-12-15 19:13:35 UTC
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/