113875 2013-04-03 07:15:02 -0700 [Gstreamer] Avoid calls to g_slist_nth_data in webKitWebAudioSrcLoop() 2013-04-24 01:22:44 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cdumez cdumez eric.carlson feature-media-reviews jer.noble laszlo.gombos mrobinson pnormand praveen.j webkit.review.bot oldest_to_newest 868697 0 cdumez 2013-04-03 07:15:02 -0700 In webKitWebAudioSrcLoop(), we iterate over 2 GSLists by using a counter and then calling g_slist_nth_data() to get the element of each GSList. This is inefficient because calling g_slist_nth_data() will iterate the GSList up until index 'n'. 868698 1 196347 cdumez 2013-04-03 07:19:30 -0700 Created attachment 196347 Patch 868700 2 196347 pnormand 2013-04-03 07:28:30 -0700 Comment on attachment 196347 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196347&action=review > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:377 > + for (padsIt = priv->pads, buffersIt = channelBufferList; padsIt && buffersIt; padsIt = padsIt->next, buffersIt = buffersIt->next) { Use g_slist_next() perhaps? 868708 3 196350 cdumez 2013-04-03 07:41:03 -0700 Created attachment 196350 Patch Use g_slist_next(). 868709 4 196350 pnormand 2013-04-03 07:44:52 -0700 Comment on attachment 196350 Patch Ok 868725 5 196350 webkit.review.bot 2013-04-03 08:07:32 -0700 Comment on attachment 196350 Patch Clearing flags on attachment: 196350 Committed r147555: <http://trac.webkit.org/changeset/147555> 868726 6 webkit.review.bot 2013-04-03 08:07:36 -0700 All reviewed patches have been landed. Closing bug. 880350 7 praveen.j 2013-04-23 23:56:29 -0700 This patch causes build error with gstreamer-0.10. Maybe including these changes under GST_API_VERSION_1 should be good in my opinion. 880354 8 pnormand 2013-04-24 00:02:19 -0700 That patch doesn't use any GStreamer 1.x-only API, I think. Can you please post the error message? 880356 9 praveen.j 2013-04-24 00:09:12 -0700 Its because of the variable 'i' in line no. 384. which was removed in this patch. /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: In function ‘void webKitWebAudioSrcLoop(WebKitWebAudioSrc*)’: /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: error: name lookup of ‘i’ changed for ISO ‘for’ scoping [-fpermissive] /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: note: (if you use ‘-fpermissive’ G++ will accept your code) 880357 10 pnormand 2013-04-24 00:11:12 -0700 So this is unrelated with GStreamer 1.x. But feel free to file a patch in a separate bug. 880358 11 cdumez 2013-04-24 00:13:07 -0700 (In reply to comment #9) > Its because of the variable 'i' in line no. 384. which was removed in this patch. > > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: In function ‘void webKitWebAudioSrcLoop(WebKitWebAudioSrc*)’: > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: error: name lookup of ‘i’ changed for ISO ‘for’ scoping [-fpermissive] > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: note: (if you use ‘-fpermissive’ G++ will accept your code) Right, then the proper fix it to add the i counter under #ifndef GST_API_VERSION_1. Do you want to do it or should I? 880359 12 pnormand 2013-04-24 00:15:50 -0700 Perhaps use g_slist_index() 880372 13 praveen.j 2013-04-24 00:27:15 -0700 How about moving setcaps related code to the first "for" loop in the function a(In reply to comment #11) > (In reply to comment #9) > > Its because of the variable 'i' in line no. 384. which was removed in this patch. > > > > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: In function ‘void webKitWebAudioSrcLoop(WebKitWebAudioSrc*)’: > > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: error: name lookup of ‘i’ changed for ISO ‘for’ scoping [-fpermissive] > > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: note: (if you use ‘-fpermissive’ G++ will accept your code) > > Right, then the proper fix it to add the i counter under #ifndef GST_API_VERSION_1. Do you want to do it or should I? How about moving setcaps related code to first "for" loop and retain your changes as it is? You may please continue with the checkin. :-) 880391 14 cdumez 2013-04-24 01:22:44 -0700 gstreamer 0.10 build fix landed in <http://trac.webkit.org/changeset/149021>. 196347 2013-04-03 07:19:30 -0700 2013-04-03 07:41:03 -0700 Patch 113875_gslist_iter.patch text/plain 2408 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAwNDZmYmFjLi40Y2NmY2RmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMg QEAKKzIwMTMtMDQtMDMgIENocmlzdG9waGUgRHVtZXogIDxjaC5kdW1lekBzaXNhLnNhbXN1bmcu Y29tPgorCisgICAgICAgIFtHc3RyZWFtZXJdIEF2b2lkIGNhbGxzIHRvIGdfc2xpc3RfbnRoX2Rh dGEgaW4gd2ViS2l0V2ViQXVkaW9TcmNMb29wKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTExMzg3NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIHdlYktpdFdlYkF1ZGlvU3JjTG9vcCgpIHdhcyBpdGVyYXRp bmcgb3ZlciAyIEdTTGlzdHMgYnkgdXNpbmcgYSBjb3VudGVyIGFuZCB0aGVuCisgICAgICAgIGNh bGxpbmcgZ19zbGlzdF9udGhfZGF0YSgpIHRvIGdldCB0aGUgZWxlbWVudCBvZiBlYWNoIEdTTGlz dC4gVGhpcyBpcyBpbmVmZmljaWVudAorICAgICAgICBiZWNhdXNlIGNhbGxpbmcgZ19zbGlzdF9u dGhfZGF0YSgpIHdpbGwgaXRlcmF0ZSB0aGUgR1NMaXN0IHVwIHVudGlsIGluZGV4ICduJy4KKwor ICAgICAgICBUaGlzIHBhdGNoIGltcHJvdmVzIHRoaXMgYnkga2VlcGluZyBwb2ludGVycyB0byB0 aGUgY3VycmVudCBlbGVtZW50IG9mIGVhY2ggbGlzdAorICAgICAgICB3aGlsZSBpdGVyYXRpbmcs IHNvIHRoYXQgd2UgY2FuIHNpbXBseSBhY2Nlc3MgZWxlbWVudC0+bmV4dCB0byBpdGVyYXRlIGlu c3RlYWQKKyAgICAgICAgb2YgZ19zbGlzdF9udGhfZGF0YSgpLgorCisgICAgICAgIE5vIG5ldyB0 ZXN0cywgbm8gYmVoYXZpb3IgY2hhbmdlLgorCisgICAgICAgICogcGxhdGZvcm0vYXVkaW8vZ3N0 cmVhbWVyL1dlYktpdFdlYkF1ZGlvU291cmNlR1N0cmVhbWVyLmNwcDoKKyAgICAgICAgKHdlYktp dFdlYkF1ZGlvU3JjTG9vcCk6CisKIDIwMTMtMDQtMDMgIEFsbGFuIFNhbmRmZWxkIEplbnNlbiAg PGFsbGFuLmplbnNlbkBkaWdpYS5jb20+CiAKICAgICAgICAgW1F0XSBNb25vc3BhY2UgZm9udCBk b2VzIG5vdCByZW5kZXIgaW4gcHJvcGVyIHNpemVzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9hdWRpby9nc3RyZWFtZXIvV2ViS2l0V2ViQXVkaW9Tb3VyY2VHU3RyZWFtZXIu Y3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vYXVkaW8vZ3N0cmVhbWVyL1dlYktpdFdlYkF1 ZGlvU291cmNlR1N0cmVhbWVyLmNwcAppbmRleCBlZGM4Y2FhLi4xOThkYWFjIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9hdWRpby9nc3RyZWFtZXIvV2ViS2l0V2ViQXVkaW9T b3VyY2VHU3RyZWFtZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2F1ZGlvL2dz dHJlYW1lci9XZWJLaXRXZWJBdWRpb1NvdXJjZUdTdHJlYW1lci5jcHAKQEAgLTM3Miw5ICszNzIs MTEgQEAgc3RhdGljIHZvaWQgd2ViS2l0V2ViQXVkaW9TcmNMb29wKFdlYktpdFdlYkF1ZGlvU3Jj KiBzcmMpCiAgICAgLy8gRklYTUU6IEFkZCBzdXBwb3J0IGZvciBsb2NhbC9saXZlIGF1ZGlvIGlu cHV0LgogICAgIHByaXYtPnByb3ZpZGVyLT5yZW5kZXIoMCwgcHJpdi0+YnVzLCBwcml2LT5mcmFt ZXNUb1B1bGwpOwogCi0gICAgZm9yIChpbnQgaSA9IGdfc2xpc3RfbGVuZ3RoKHByaXYtPnBhZHMp IC0gMTsgaSA+PSAwOyBpLS0pIHsKLSAgICAgICAgR3N0UGFkKiBwYWQgPSBzdGF0aWNfY2FzdDxH c3RQYWQqPihnX3NsaXN0X250aF9kYXRhKHByaXYtPnBhZHMsIGkpKTsKLSAgICAgICAgR3N0QnVm ZmVyKiBjaGFubmVsQnVmZmVyID0gc3RhdGljX2Nhc3Q8R3N0QnVmZmVyKj4oZ19zbGlzdF9udGhf ZGF0YShjaGFubmVsQnVmZmVyTGlzdCwgaSkpOworICAgIEdTTGlzdCogcGFkc0l0OworICAgIEdT TGlzdCogYnVmZmVyc0l0OworICAgIGZvciAocGFkc0l0ID0gcHJpdi0+cGFkcywgYnVmZmVyc0l0 ID0gY2hhbm5lbEJ1ZmZlckxpc3Q7IHBhZHNJdCAmJiBidWZmZXJzSXQ7IHBhZHNJdCA9IHBhZHNJ dC0+bmV4dCwgYnVmZmVyc0l0ID0gYnVmZmVyc0l0LT5uZXh0KSB7CisgICAgICAgIEdzdFBhZCog cGFkID0gc3RhdGljX2Nhc3Q8R3N0UGFkKj4ocGFkc0l0LT5kYXRhKTsKKyAgICAgICAgR3N0QnVm ZmVyKiBjaGFubmVsQnVmZmVyID0gc3RhdGljX2Nhc3Q8R3N0QnVmZmVyKj4oYnVmZmVyc0l0LT5k YXRhKTsKIAogI2lmbmRlZiBHU1RfQVBJX1ZFUlNJT05fMQogICAgICAgICBHUmVmUHRyPEdzdENh cHM+IG1vbm9DYXBzID0gYWRvcHRHUmVmKGdldEdTdHJlYW1lck1vbm9BdWRpb0NhcHMocHJpdi0+ c2FtcGxlUmF0ZSkpOwo= 196350 2013-04-03 07:41:03 -0700 2013-04-03 08:07:31 -0700 Patch 113875_gslist_iter.patch text/plain 2422 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAwNDZmYmFjLi41YjcwOGFmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMg QEAKKzIwMTMtMDQtMDMgIENocmlzdG9waGUgRHVtZXogIDxjaC5kdW1lekBzaXNhLnNhbXN1bmcu Y29tPgorCisgICAgICAgIFtHc3RyZWFtZXJdIEF2b2lkIGNhbGxzIHRvIGdfc2xpc3RfbnRoX2Rh dGEgaW4gd2ViS2l0V2ViQXVkaW9TcmNMb29wKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTExMzg3NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIHdlYktpdFdlYkF1ZGlvU3JjTG9vcCgpIHdhcyBpdGVyYXRp bmcgb3ZlciAyIEdTTGlzdHMgYnkgdXNpbmcgYSBjb3VudGVyIGFuZCB0aGVuCisgICAgICAgIGNh bGxpbmcgZ19zbGlzdF9udGhfZGF0YSgpIHRvIGdldCB0aGUgZWxlbWVudCBvZiBlYWNoIEdTTGlz dC4gVGhpcyBpcyBpbmVmZmljaWVudAorICAgICAgICBiZWNhdXNlIGNhbGxpbmcgZ19zbGlzdF9u dGhfZGF0YSgpIHdpbGwgaXRlcmF0ZSB0aGUgR1NMaXN0IHVwIHVudGlsIGluZGV4ICduJy4KKwor ICAgICAgICBUaGlzIHBhdGNoIGltcHJvdmVzIHRoaXMgYnkga2VlcGluZyBwb2ludGVycyB0byB0 aGUgY3VycmVudCBlbGVtZW50IG9mIGVhY2ggbGlzdAorICAgICAgICB3aGlsZSBpdGVyYXRpbmcs IHNvIHRoYXQgd2UgY2FuIHNpbXBseSB1c2UgZ19zbGlzdF9uZXh0KCkgdG8gaXRlcmF0ZSBpbnN0 ZWFkCisgICAgICAgIG9mIGdfc2xpc3RfbnRoX2RhdGEoKS4KKworICAgICAgICBObyBuZXcgdGVz dHMsIG5vIGJlaGF2aW9yIGNoYW5nZS4KKworICAgICAgICAqIHBsYXRmb3JtL2F1ZGlvL2dzdHJl YW1lci9XZWJLaXRXZWJBdWRpb1NvdXJjZUdTdHJlYW1lci5jcHA6CisgICAgICAgICh3ZWJLaXRX ZWJBdWRpb1NyY0xvb3ApOgorCiAyMDEzLTA0LTAzICBBbGxhbiBTYW5kZmVsZCBKZW5zZW4gIDxh bGxhbi5qZW5zZW5AZGlnaWEuY29tPgogCiAgICAgICAgIFtRdF0gTW9ub3NwYWNlIGZvbnQgZG9l cyBub3QgcmVuZGVyIGluIHByb3BlciBzaXplcwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vYXVkaW8vZ3N0cmVhbWVyL1dlYktpdFdlYkF1ZGlvU291cmNlR1N0cmVhbWVyLmNw cCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2F1ZGlvL2dzdHJlYW1lci9XZWJLaXRXZWJBdWRp b1NvdXJjZUdTdHJlYW1lci5jcHAKaW5kZXggZWRjOGNhYS4uZDdlZjk0MiAxMDA2NDQKLS0tIGEv U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vYXVkaW8vZ3N0cmVhbWVyL1dlYktpdFdlYkF1ZGlvU291 cmNlR1N0cmVhbWVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9hdWRpby9nc3Ry ZWFtZXIvV2ViS2l0V2ViQXVkaW9Tb3VyY2VHU3RyZWFtZXIuY3BwCkBAIC0zNzIsOSArMzcyLDEx IEBAIHN0YXRpYyB2b2lkIHdlYktpdFdlYkF1ZGlvU3JjTG9vcChXZWJLaXRXZWJBdWRpb1NyYyog c3JjKQogICAgIC8vIEZJWE1FOiBBZGQgc3VwcG9ydCBmb3IgbG9jYWwvbGl2ZSBhdWRpbyBpbnB1 dC4KICAgICBwcml2LT5wcm92aWRlci0+cmVuZGVyKDAsIHByaXYtPmJ1cywgcHJpdi0+ZnJhbWVz VG9QdWxsKTsKIAotICAgIGZvciAoaW50IGkgPSBnX3NsaXN0X2xlbmd0aChwcml2LT5wYWRzKSAt IDE7IGkgPj0gMDsgaS0tKSB7Ci0gICAgICAgIEdzdFBhZCogcGFkID0gc3RhdGljX2Nhc3Q8R3N0 UGFkKj4oZ19zbGlzdF9udGhfZGF0YShwcml2LT5wYWRzLCBpKSk7Ci0gICAgICAgIEdzdEJ1ZmZl ciogY2hhbm5lbEJ1ZmZlciA9IHN0YXRpY19jYXN0PEdzdEJ1ZmZlcio+KGdfc2xpc3RfbnRoX2Rh dGEoY2hhbm5lbEJ1ZmZlckxpc3QsIGkpKTsKKyAgICBHU0xpc3QqIHBhZHNJdDsKKyAgICBHU0xp c3QqIGJ1ZmZlcnNJdDsKKyAgICBmb3IgKHBhZHNJdCA9IHByaXYtPnBhZHMsIGJ1ZmZlcnNJdCA9 IGNoYW5uZWxCdWZmZXJMaXN0OyBwYWRzSXQgJiYgYnVmZmVyc0l0OyBwYWRzSXQgPSBnX3NsaXN0 X25leHQocGFkc0l0KSwgYnVmZmVyc0l0ID0gZ19zbGlzdF9uZXh0KGJ1ZmZlcnNJdCkpIHsKKyAg ICAgICAgR3N0UGFkKiBwYWQgPSBzdGF0aWNfY2FzdDxHc3RQYWQqPihwYWRzSXQtPmRhdGEpOwor ICAgICAgICBHc3RCdWZmZXIqIGNoYW5uZWxCdWZmZXIgPSBzdGF0aWNfY2FzdDxHc3RCdWZmZXIq PihidWZmZXJzSXQtPmRhdGEpOwogCiAjaWZuZGVmIEdTVF9BUElfVkVSU0lPTl8xCiAgICAgICAg IEdSZWZQdHI8R3N0Q2Fwcz4gbW9ub0NhcHMgPSBhZG9wdEdSZWYoZ2V0R1N0cmVhbWVyTW9ub0F1 ZGlvQ2Fwcyhwcml2LT5zYW1wbGVSYXRlKSk7Cg==