cocos2d / cocos2d-js

cocos2d-x for JS
http://www.cocos2d-x.org
MIT License
1.86k stars 490 forks source link

Make Downloader::getContentSize asynchronous #1217

Closed pandamicro closed 9 years ago

pandamicro commented 9 years ago

jsb.load_remote_img block the main thread a while to check the file size.

0xJimmyBeh commented 9 years ago

@pandamicro

Is there any milestone for this enhancement in the coming release?

Or is there any temporarily solution for this blocking main thread problem?

Thanks.

ghost commented 9 years ago

Any idea when is this going to be fixed, cc.loader.loadImg blocks main thread even loading local imgs!

pandamicro commented 9 years ago

Loading local imgs won't use the downloader to check file size, so it's not the same issue. But if your local images are too large, it will take a little while to load the texture into memory. You can use cc.textureCache.addImageAsync which is asynchonous.

0xJimmyBeh commented 9 years ago

Why there is no cc.textureCache.addImageAsync API show in the doc?

http://www.cocos2d-x.org/reference/html5-js/V3.3/symbols/cc.textureCache.html

So any idea how to load online images asynchronous?

Thanks.

pandamicro commented 9 years ago

Oh, you are right, it was not well considered, I created an issue here: https://github.com/cocos2d/cocos2d-js/issues/1507

Load online images is asynchronous, which I describe in the current issue is that the getContentSize API is synchronous so it blocks sometimes for just a tiny amount of time.

0xJimmyBeh commented 9 years ago

I understand the issue is on getContentSize as you described.

To make my question more precise, i should ask "How to load online images Fully asynchronous" as getContentSize API is part of the code.

And thanks for the info in the issue https://github.com/cocos2d/cocos2d-js/issues/1507.

Is it i just need to make the change as below to the frameworks/js-bindings/bindings/script/jsb_boot.js to solve the blocking issue? https://github.com/pandamicro/cocos2d-js/commit/2a124b5aec2444e85a54981f271ee47b3e14b314

pandamicro commented 9 years ago

pandamicro@2a124b5 will only solve the issue of @vktrsm

I will find some time to do this issue as soon as possible, sorry for keeping you guys waiting

0xJimmyBeh commented 9 years ago

I'm looking forward to hearing good news from you.

Thanks for your effort!

ghost commented 9 years ago

@pandamicro Thx for the fix!! We didnt understand why addImgAsync was removed on 3.0 (was working in 2.2, docs say is merged into addImage and makes async load when callback is provided, but this was untrue), thats why I was trying to use loadImg.

Again thanks.

thangenius commented 9 years ago

I've made a workaround for this issue by modifying the below code to jsbinding/binding/manual/extensions/jsb_cocos2dx_extension_manual.cpp (cocos2d-js 3.2):

  1. line 971, change download(..) function as follow void JSDownloaderDelegator::download(JSContext cx, JSObject obj, const std::string &url, const jsval &callback) { auto t = std::thread([cx, obj, url, callback]() { new JSDownloaderDelegator(cx, obj, url, callback); }); t.detach(); }
  2. line 900 of constructor JSDownloaderDelegator::JSDownloaderDelegator(), change _downloader->downloadToBufferASync(_url, _buffer, _size); to _downloader->downloadToBufferSync(_url, _buffer, _size); (make this change to avoid another thread to be created to download)
  3. Finally, include std::thread in the include section

    include

Hope this helps while waiting for the official fix :)

pandamicro commented 9 years ago

Thanks @thangenius and sorry for being late Here is the fix: https://github.com/cocos2d/cocos2d-js/pull/1582

0xJimmyBeh commented 9 years ago

@thangenius Your solution is working!

@pandamicro Finally you have the time to fix this problem! Thanks for the effort!

I have try the changes with Cocos2d-JS v3.3 It does solved the problem for calling the load online image function at start.

But when the online image success loaded with cc.loader.loadImg, it still cause the app to freeze for awhile. I tested with just showing cc.log.

cc.loader.loadImg is called multiple times to load different online image at the same time.

cc.loader.loadImg(imageUrl, {isCrossOrigin : true}, function(err, texture) { cc.log("success");// this indicated the image loaded and the app freeze for awhile when saw this log in console. }

I have done the changes as this https://github.com/cocos2d/cocos2d-js/issues/1507 but same.

The freeze is noticeable on Android and iOS, but on Mac, it just has very very short time freeze and can be ignored.

Thanks.

thangenius commented 9 years ago

@pandamicro thanks for the official fix! I will apply it for my project instead of my workaround.

@Zinitter I am not sure if the underlying http library of Downloader limits the number of concurrent http connection or not. If it does not, I think the problem is too many http connections being opened and do read operations at the same time causes the freeze, especially on mobile platform. Have you tried to put the image loading tasks to a queue (an array) and implement an executor who pop only a limited number of tasks at a same time from queue, do loading and repeat until the queue is empty (and may start back if there are more tasks put to queue)?. Try to keep the number of poped tasks at a same time as low as possible on mobile platform (I suggest 1 which means loading sequentially), on desktop, this number can be large. Hope this help you.

0xJimmyBeh commented 9 years ago

@thangenius

Thanks for your suggestion.

I have tested to call cc.loader.loadImg only once to load an online image, the app still freeze when the image success loaded.

The freeze might cause by the callback of cc.loader.loadImg.

pandamicro commented 9 years ago

@Zinitter Normally, the header request and download process are now totally asynchronous. Can you post your implementation so that we can analyze the reason ?

Make sure also you have applied changes in https://github.com/cocos2d/cocos2d-js/pull/1508/files

0xJimmyBeh commented 9 years ago

Here is the simple sample code :

loadImage : function()
{
        cc.loader.loadImg("http://discuss.cocos2d-x.org/images/logo-vertical.png", {isCrossOrigin : true}, function(err, texture)
        {
            cc.log("image loaded");

            var sprite = new cc.Sprite(texture);
            sprite.setPosition((winSize.width*0.50), (winSize.height*0.50));
            self.addChild(sprite);
        });
}

When touch a cc.MenuItemSprite, it call this loadImage function.

I tested by scrolling the scene, the scene will not move for a while when the image loaded.

pandamicro commented 9 years ago

Can you print some log to see where the freeze is happening ?

loadImg : function(url, option, cb){
    var l = arguments.length;
    if(l == 2) cb = option;

    var cachedTex = cc.textureCache.getTextureForKey(url);
    if (cachedTex) {
        cb && cb(null, cachedTex);
    }
    else if (url.match(jsb.urlRegExp)) {
        cc.log("1");
        jsb.loadRemoteImg(url, function(succeed, tex) {
            cc.log("2");
            if (succeed) {
                cb && cb(null, tex);
            }
            else {
                cb && cb("Load image failed");
            }
        });
    }
    else {
        cc.textureCache._addImageAsync(url, function (tex){
            if (tex instanceof cc.Texture2D)
                cb && cb(null, tex);
            else cb && cb("Load image failed");
        });
    }
},
0xJimmyBeh commented 9 years ago

@pandamicro

I tested with your code.

The freeze happen when showing "2" in log. Mean it freeze a while then only print the "2" in log. As i noticed, it happen when jsb.loadRemoteImg success loaded image and calling the callback.

There is no freeze before and after "1" in log.

p/s : The freeze only happen on testing in Android and iOS devices. When testing on Mac runtime, the freeze is very short and can be ignored.

Some question to make sure the patch take effect :

If patch applied to the file in js-bindings/bindings/manual and js-bindings/cocos2d-x/ Is it just need to rebuild a new runtime to make the changes take effect?

If patch applied to the file in js-bindings/bindings/auto/ Then need to run the tojs/genbindings.py?

Thanks.

pandamicro commented 9 years ago

I don't understand, you mean like this ?

1 -> 2 -> freeze -> image show up

If you want to know whether the runtime have successfully updated, you can print logs in the C++ logic.

0xJimmyBeh commented 9 years ago

It is like this : 1 -> free time -> (freeze just before 2) -> 2 - > image show up

It is because loading the online image take time, so there is free time between 1 -> 2

More specifically not freeze -> 1 -> not freeze(when waiting the online image to load) -> freeze(image loaded) -> 2 -> image show up

pandamicro commented 9 years ago

Sorry guys, I made a mistake, the real reason that the download process was frozen is not only the download process, it's also caused by the Image::initWithImageData function, it reads a char * buffer and create an Image object. This process takes time on mobile platforms.

So I'm reverting my previous solutions and finally I think @thangenius had the right solution. It's just a problem about when to start another thread.

My previous solution start one thread to check the size and then a second thread to download. Even though, I can't go through the problem that we don't have a asynchronous API to init image data. And @thangenius 's solution only start one thread, I'm making the content size check and download process synchronized in the thread.

Thanks @thangenius for proposing the solution. Thanks @Zinitter for sticking on this issue.

thangenius commented 9 years ago

You welcome @pandamicro :) I've also opened a new issue which is also related to asynchronous remote image loading (#1589) . Please take a look at it because it cause crash to the game. Thanks!

0xJimmyBeh commented 9 years ago

@pandamicro

Thanks for your effort to find out the real frozen issue. So the frozen problem is half solved.

Do we must invoke Image::initWithImageData after the image data downloaded to buffer?

Update : Why the frozen issue is not happen on Mac but only Android and iOS? Are they process the imageData differently?

Thanks.

pandamicro commented 9 years ago

Do we must invoke Image::initWithImageData after the image data downloaded to buffer?

For image data downloaded to buffer, yes.
For image downloaded to file, no, we can use other APIs.

Why the frozen issue is not happen on Mac but only Android and iOS? Are they process the imageData differently?

CCImage implementation is in the platform folder, so it's platform related and its performance may vary between different platforms
0xJimmyBeh commented 9 years ago

What do you mean by image downloaded to file?Could we use this to solve the frozen problem?

Thanks for your info, i know more about the folder structure.

This issue is closed mean the frozen issue won't be solved soon?

thangenius commented 9 years ago

@Zinitter It has been solved, apply the changes here https://github.com/cocos2d/cocos2d-js/pull/1597 or follow my workaround above. But you better apply the changes in #1597 because it also fixes the crash issue #1589

0xJimmyBeh commented 9 years ago

@thangenius As this issue 1217 Make Downloader::getContentSize asynchronous is solved.

I mean the frozen issue caused by Image::initWithImageData in Android and iOS.

thangenius commented 9 years ago

@Zinitter the fix for issue #1597 means the operation Image::initWithImageData also run in another thread so it will not block (freeze) the render thread anymore. Have you tried to apply it yet?

0xJimmyBeh commented 9 years ago

I see.

I tried to apply only this patch https://github.com/cocos2d/cocos2d-js/pull/1597/files but i can't rebuild the runtime.

There are some error on js-bindings/bindings/manual/extension/jsb_cocos2dx_extension_manual.cpp

How should apply this patch? Do i need to apply other patch before this?

Thanks.