Bug 204673 - [GStreamer] Convert custom GObject subclasses to WEBKIT_DEFINE_TYPE
Summary: [GStreamer] Convert custom GObject subclasses to WEBKIT_DEFINE_TYPE
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Víctor M. Jáquez L.
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-28 03:08 PST by Philippe Normand
Modified: 2020-09-13 05:08 PDT (History)
15 users (show)

See Also:


Attachments
Patch (22.12 KB, patch)
2020-09-04 08:47 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (22.23 KB, patch)
2020-09-07 02:42 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (6.42 KB, patch)
2020-09-08 03:19 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2019-11-28 03:08:51 PST
Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp
40:G_DEFINE_TYPE(GstAllocatorFastMalloc, gst_allocator_fast_malloc, GST_TYPE_ALLOCATOR)

Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp
45:G_DEFINE_TYPE_WITH_CODE(WebKitTextCombiner, webkit_text_combiner, GST_TYPE_BIN,
75:G_DEFINE_TYPE(WebKitTextCombinerPad, webkit_text_combiner_pad, GST_TYPE_GHOST_PAD);

Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp
35:G_DEFINE_TYPE_WITH_CODE(WebKitTextSink, webkit_text_sink, GST_TYPE_APP_SINK,

Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp
141:G_DEFINE_TYPE_WITH_CODE(WebKitVideoSink, webkit_video_sink, GST_TYPE_VIDEO_SINK, GST_DEBUG_CATEGORY_INIT(webkitVideoSinkDebug, "webkitsink", 0, "webkit video sink"));

Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp
65:G_DEFINE_TYPE(WebKitMediaClearKeyDecrypt, webkit_media_clear_key_decrypt, WEBKIT_TYPE_MEDIA_CENC_DECRYPT);

Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp
188:G_DEFINE_TYPE_WITH_CODE(WebKitMediaSrc, webkit_media_src, GST_TYPE_BIN,

Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp
60:G_DEFINE_TYPE(WebKitMediaCommonEncryptionDecrypt, webkit_media_common_encryption_decrypt, GST_TYPE_BASE_TRANSFORM);

Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp
136:G_DEFINE_TYPE_WITH_CODE(WebKitWebAudioSrc, webkit_web_audio_src, GST_TYPE_BIN, GST_DEBUG_CATEGORY_INIT(webkit_web_audio_src_debug, \

Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp
332:G_DEFINE_TYPE_WITH_CODE(WebKitMediaStreamSrc, webkit_media_stream_src, GST_TYPE_BIN, doInit);

Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp
92:G_DEFINE_TYPE_WITH_PRIVATE (GstWebrtcVideoEncoder, gst_webrtc_video_encoder,
Comment 1 Víctor M. Jáquez L. 2020-09-04 08:47:50 PDT
Created attachment 407980 [details]
Patch
Comment 2 Víctor M. Jáquez L. 2020-09-04 08:51:59 PDT
I didn't changed GstAllocatorFastMalloc, TextCombinerGStreamer nor TextSinkGStreamer, since they don't use private structures, so there's no case to use the macro. 


GStreamerVideoEncoder it's a bit complex since it doesn't use the private structure as the macro expects it. I'll work on this class later.

GStreamerMediaStreamSource is too complex to me to try to try modify it, perhaps the macro doesn't worth it for it.
Comment 3 Philippe Normand 2020-09-06 11:36:40 PDT
(In reply to Víctor M. Jáquez L. from comment #2)
> I didn't changed GstAllocatorFastMalloc, TextCombinerGStreamer nor
> TextSinkGStreamer, since they don't use private structures, so there's no
> case to use the macro. 
> 
> 
> GStreamerVideoEncoder it's a bit complex since it doesn't use the private
> structure as the macro expects it. I'll work on this class later.
> 

If the intent is still to upstream this to GStreamer it should keep using G_DEFINE_TYPE.
Comment 4 Philippe Normand 2020-09-07 01:31:53 PDT
Comment on attachment 407980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407980&action=review

LGTM, just a nit, valid for all new calls of WEBKIT_DEFINE_TYPE:

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitThunderDecryptorGStreamer.cpp:65
> +WEBKIT_DEFINE_TYPE(WebKitMediaThunderDecrypt, webkit_media_thunder_decrypt, WEBKIT_TYPE_MEDIA_CENC_DECRYPT);

I think there's no need for ending ;
Comment 5 Víctor M. Jáquez L. 2020-09-07 02:31:25 PDT
(In reply to Philippe Normand from comment #3)
> (In reply to Víctor M. Jáquez L. from comment #2)
> > I didn't changed GstAllocatorFastMalloc, TextCombinerGStreamer nor
> > TextSinkGStreamer, since they don't use private structures, so there's no
> > case to use the macro. 
> > 
> > 
> > GStreamerVideoEncoder it's a bit complex since it doesn't use the private
> > structure as the macro expects it. I'll work on this class later.
> > 
> 
> If the intent is still to upstream this to GStreamer it should keep using
> G_DEFINE_TYPE.

I don't think there's an intent to upstream it. I'd need to ask Thibault.
Comment 6 Víctor M. Jáquez L. 2020-09-07 02:42:14 PDT
Created attachment 408170 [details]
Patch
Comment 7 EWS 2020-09-07 03:09:20 PDT
Committed r266697: <https://trac.webkit.org/changeset/266697>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408170 [details].
Comment 8 Radar WebKit Bug Importer 2020-09-07 03:10:14 PDT
<rdar://problem/68455348>
Comment 9 Carlos Garcia Campos 2020-09-08 01:22:39 PDT
Comment on attachment 408170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408170&action=review

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:84
> +        provider = nullptr;
> +        bus = nullptr;

nullptr initializations aren't needed for private structs, since they are filled with 0 on allocation by GLib.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:194
> +    g_type_class_add_private(webKitWebAudioSrcClass, sizeof(WebKitWebAudioSrcPrivate));

This is already called by WEBKIT_DEFINE_TYPE, now it's called twice, which causes a runtime warning.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:280
>      g_type_class_add_private(klass, sizeof(WebKitVideoSinkPrivate));

You should remove this one too.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:88
>      g_type_class_add_private(klass, sizeof(WebKitMediaClearKeyDecryptPrivate));

And this.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:81
>      g_type_class_add_private(klass, sizeof(WebKitMediaCommonEncryptionDecryptPrivate));

And this.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitThunderDecryptorGStreamer.cpp:107
>      g_type_class_add_private(klass, sizeof(WebKitMediaThunderDecryptPrivate));

And this.
Comment 10 Víctor M. Jáquez L. 2020-09-08 03:18:59 PDT
Reopening to attach new patch.
Comment 11 Víctor M. Jáquez L. 2020-09-08 03:19:01 PDT
Created attachment 408220 [details]
Patch
Comment 12 EWS 2020-09-08 06:18:52 PDT
Committed r266721: <https://trac.webkit.org/changeset/266721>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408220 [details].
Comment 13 Philippe Normand 2020-09-13 05:08:43 PDT
https://trac.webkit.org/r266992