Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Downloader::getContentSize asynchronous #1217

Closed
pandamicro opened this issue Dec 17, 2014 · 29 comments
Closed

Make Downloader::getContentSize asynchronous #1217

pandamicro opened this issue Dec 17, 2014 · 29 comments
Assignees
Milestone

Comments

@pandamicro
Copy link
Contributor

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

@pandamicro pandamicro self-assigned this Dec 17, 2014
@0xJimmyBeh
Copy link

@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
Copy link

ghost commented Mar 3, 2015

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

@pandamicro
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

Oh, you are right, it was not well considered, I created an issue here:
#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
Copy link

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 #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?
pandamicro@2a124b5

@pandamicro
Copy link
Contributor Author

pandamicro/cocos2d-js@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

@pandamicro pandamicro added this to the v3.4 milestone Mar 4, 2015
@0xJimmyBeh
Copy link

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

Thanks for your effort!

@ghost
Copy link

ghost commented Mar 5, 2015

@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
Copy link

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 added a commit to pandamicro/cocos2d-x that referenced this issue Mar 24, 2015
pandamicro added a commit to cocos2d-html5/cocos2d-x that referenced this issue Mar 24, 2015
cocos2d/cocos2d-js#1217: Add Downloader::getContentSizeAsync to improveremote image loading
@pandamicro
Copy link
Contributor Author

Thanks @thangenius and sorry for being late
Here is the fix: #1582

pandamicro added a commit that referenced this issue Mar 24, 2015
Fixed #1217: make remote image loading fully asynchonous
@0xJimmyBeh
Copy link

@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 #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
Copy link

@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
Copy link

@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
Copy link
Contributor Author

@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

pandamicro added a commit to pandamicro/cocos2d-x that referenced this issue Mar 25, 2015
pandamicro added a commit to cocos2d-html5/cocos2d-x that referenced this issue Mar 25, 2015
pandamicro added a commit to pandamicro/cocos2d-js that referenced this issue Mar 25, 2015
pandamicro added a commit that referenced this issue Mar 25, 2015
#1217: Improved jsb.loadRemoteImg implementation
@0xJimmyBeh
Copy link

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
Copy link
Contributor Author

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
Copy link

@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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link

@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
Copy link
Contributor Author

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
Copy link

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
Copy link

@Zinitter It has been solved, apply the changes here #1597 or follow my workaround above. But you better apply the changes in #1597 because it also fixes the crash issue #1589

@0xJimmyBeh
Copy link

@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
Copy link

@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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants