Move tile caching code inside tilecache.js. #659

avandecreme · 2015-05-29T16:00:17Z

I was looking on how to handle modifying the tiles in an asynchronous way in case the operation is lengthy.
That would also allow interaction with http://camanjs.com/.

The idea would be to provide a way to modify a tile before it is stored in cache. Probably some sort of callback or event inside onTileLoad in tiledimage.js

This first commit is just some cleanup. I suspect it might also save a bit of memory because the ImageRecord was keeping a reference to the image even if it has been rendered.
I deleted a method, hopefully this is internal enough for not being a breaking change.

avandecreme · 2015-05-29T19:21:20Z

Ok this second commit actually add a tile-loaded event.
However, that is not enough for my purpose.
I would need some promise system so that the tile is added to the cache only when all the event handlers are done.

Any idea on that?

Here is how I am using this with Caman (I know I am currently wasting memory :) )

osd.addHandler( "tile-loaded", function ( event ) {
     var tile = event.tile;
     var tiledImage = event.tiledImage;
     var image = event.image;
     var canvas = document.createElement( 'canvas' );
     canvas.width = image.width;
     canvas.height = image.height;
     tile._renderedContext = canvas.getContext( '2d' );
     tile._renderedContext.drawImage( image, 0, 0 );
     Caman( canvas, function () {
         this.threshold( 100 );
         this.render( function () {
             // This function should just resolve the promise
             // Right now, I need to comment out everything in setTileLoaded except the event raising.
             tile.loading = false;
             tile.loaded = true;
             var cutoff = Math.ceil( Math.log(
                 tiledImage.source.getTileSize( tile.level ) ) / Math.log( 2 ) );
             tiledImage._tileCache.cacheTile( {
                 image: image,
                 tile: tile,
                 cutoff: cutoff,
                 tiledImage: tiledImage
             } );
             tiledImage._needsDraw = true;
         } );
     } );
 } );

 osd.addHandler( "tile-drawing", function ( event ) {
     var tile = event.tile;
     var rendered = event.rendered;
     var imgData = tile._renderedContext.getImageData( 0, 0,
         tile._renderedContext.canvas.width,
         tile._renderedContext.canvas.height);
     rendered.putImageData( imgData, 0, 0 );
 } );

iangilman · 2015-06-01T16:28:04Z

src/tiledimage.js

@@ -863,12 +861,8 @@ function updateTile( tiledImage, drawLevel, haveDrawn, x, y, level, levelOpacity
        var imageRecord = tiledImage._tileCache.getImageRecord(tile.url);
        if (imageRecord) {
            tile.loaded = true;


Probably don't need this line, since it's in setTileLoaded.

iangilman · 2015-06-01T16:41:05Z

Great idea!

Regarding the need for something like promises, I'm hesitant to actually include a promise polyfill, so maybe we should come up with something lightweight that serves our needs. Off the top of my head, perhaps the event could include a function you can call to signal you need more time. Something like this:

osd.addHandler( "tile-loaded", function ( event ) {
    var complete = event.getCompletionCallback();
    ...
    Caman( canvas, function () {
        ...
        this.render( function () {
            complete();
        };
    });
});

If during the handler you don't call getCompletionCallback, it's assumed you don't need the extra time. What do you think?

Just another note: before we land this, please make sure these changes don't have an adverse affect on the non-canvas pipeline. They shouldn't, of course, but it's good to be sure!

avandecreme · 2015-06-01T17:48:24Z

Regarding the need for something like promises, I'm hesitant to actually include a promise polyfill

Yes, I had the same thought.

If during the handler you don't call getCompletionCallback, it's assumed you don't need the extra time. What do you think?

That could work I think. That means that we need to keep track of the number of times the getCompletionCallback method is called so that the actual callback is only called when all handlers are done.
That also means that if an handler has 2 asyncs methods to call, it should do something like this:

osd.addHandler( "tile-loaded", function ( event ) {
    var camanComplete = event.getCompletionCallback();
    Caman( canvas, function () {
        ...
        this.render( function () {
            camanComplete();
        };
    });

    var anotherComplete = event.getCompletionCallback();
    otherAsyncMethod( function successCallback () {
        anotherComplete();
    });
});

Correct?

Just another note: before we land this, please make sure these changes don't have an adverse affect on the non-canvas pipeline. They shouldn't, of course, but it's good to be sure!

When I compared before/after my patch the non-canvas method was not great in both cases. I am getting black rectangles where tiles are not yet loaded. Is that expected?

iangilman · 2015-06-02T16:05:41Z

Yes, counting the number of times it's called would be necessary. Being able to call it multiple times from the same handler is a nice bonus!

No, black rectangles aren't expected...sounds like maybe we've had some degradation on the non-canvas path. Could you file an issue for it?

avandecreme · 2015-06-02T17:58:02Z

I just created the issue #661 regarding useCanvas = false.

Regarding this pull request, I am getting good results with it.
I want to study if I can now modify the effect after tiles have been loaded but if I need any other hook, I guess that can be done in another PR.

iangilman · 2015-06-03T16:59:55Z

src/tiledimage.js

@@ -989,6 +975,53 @@ function onTileLoad( tiledImage, tile, time, image ) {
    tiledImage._needsDraw = true;


I think this needsDraw happens too soon now...it should go inside the completionCallback of setTileLoaded, so it happens after the tile is actually truly loaded.

iangilman · 2015-06-03T17:08:30Z

Just some little details at this point...

…penseadragon#661

avandecreme · 2015-06-08T15:29:33Z

I just realized it would be nice to have a tile-unloaded event to allow the user to clean up the memory he allocated in tile-loaded.

If we want to be consistent with the tile-loaded event, it should be triggered on the viewer as well.
However the tile.unload method has no reference to the viewer.
So maybe we can add that code inside tilecache._unloadTile. We will need to add the tiledImage and image as parameter of this method so that we can trigger the event with the same parameters than the tile-loaded event.
I am not sure if the completion call back would make sense though. I can't think of any use case for it with the unload event.

iangilman · 2015-06-10T21:29:08Z

src/tiledimage.js

+     * @property {OpenSeadragon.Tile} tile - The tile which has been loaded.
+     * @property {function} getCompletionCallback - A function giving a callback to call
+     * when the asynchronous processing of the image is done. The image will be
+     * marked as .entirely loaded when the callback has been called once for each


You've got a stray period before entirely.

iangilman · 2015-06-10T21:40:21Z

Your proposal for tile-unloaded sounds good to me. No worries adding the extra parameters, since it's a private function. I agree it doesn't need any async features.

avandecreme · 2015-06-11T14:51:55Z

.editorconfig

@@ -1,7 +1,8 @@
 # editorconfig.org
 root = true

-[*]
+# We need to specify each folder specifically to avoid including test/lib and test/data
+[{Gruntfile.js,src/**,test/*,test/demo/**,test/helpers/**,test/modules/**}]


This is totally unrelated to the rest of the PR but the content of test/lib and test/data was keeping getting modified due to editorconfig. So I had to do something about it.

Cool; good to fix then!

iangilman · 2015-06-12T22:42:08Z

Looks lovely! @avandecreme Are you ready for it to land? Looks ready to me...

avandecreme · 2015-06-12T23:03:31Z

Yes looks good to me as well!

Move tile caching code inside tilecache.js.

iangilman · 2015-06-12T23:23:36Z

Awesome; merged. I'm looking forward to how people use this new feature!

avandecreme · 2015-06-13T00:58:25Z

I am hoping to provide a filter plugin using it soon :)

On Fri, Jun 12, 2015 at 7:23 PM, Ian Gilman notifications@github.com
wrote:

Awesome; merged. I'm looking forward to how people use this new feature!

—
Reply to this email directly or view it on GitHub
#659 (comment)
.

iangilman · 2015-06-15T16:14:24Z

Awesome :)

avandecreme added 2 commits May 29, 2015 11:49

Move tile caching code inside tilecache.js.

2538f20

Add tile-loaded event.

eda47e6

iangilman reviewed Jun 1, 2015
View reviewed changes

Fix Ian's comments.

4c1d0f9

Add getCompletionCallback method.

be657c0

iangilman reviewed Jun 3, 2015
View reviewed changes

avandecreme added 4 commits June 3, 2015 14:34

Fix flickering tiles with useCanvas=false when no cache is used. Fix o…

2674104

…penseadragon#661

Integrate Ian's comments.

d5c431b

Add tests for tile-loaded event.

7e39a52

Merge upstream.

606d25d

iangilman reviewed Jun 10, 2015
View reviewed changes

Fix editorconfig config.

fa868a0

avandecreme reviewed Jun 11, 2015
View reviewed changes

avandecreme added 4 commits June 11, 2015 10:59

Doc and test messages fix.

584daea

Add tile-unloaded event.

8a6177b

Add tile-unloaded event to changelog.

9e1433d

Fix and add unit tests.

d671049

iangilman added a commit that referenced this pull request Jun 12, 2015

Merge pull request #659 from avandecreme/master

a318318

Move tile caching code inside tilecache.js.

iangilman merged commit a318318 into openseadragon:master Jun 12, 2015

iangilman mentioned this pull request Aug 31, 2015

Rasterising SVG to image tiles instead of to the window openseadragon/svg-overlay#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move tile caching code inside tilecache.js. #659

Move tile caching code inside tilecache.js. #659

avandecreme commented May 29, 2015

avandecreme commented May 29, 2015

iangilman Jun 1, 2015

iangilman commented Jun 1, 2015

avandecreme commented Jun 1, 2015

iangilman commented Jun 2, 2015

avandecreme commented Jun 2, 2015

iangilman Jun 3, 2015

iangilman commented Jun 3, 2015

avandecreme commented Jun 8, 2015

iangilman Jun 10, 2015

iangilman commented Jun 10, 2015

avandecreme Jun 11, 2015

iangilman Jun 11, 2015

iangilman commented Jun 12, 2015

avandecreme commented Jun 12, 2015

iangilman commented Jun 12, 2015

avandecreme commented Jun 13, 2015

iangilman commented Jun 15, 2015

		@@ -989,6 +975,53 @@ function onTileLoad( tiledImage, tile, time, image ) {
		tiledImage._needsDraw = true;

Move tile caching code inside tilecache.js. #659

Move tile caching code inside tilecache.js. #659

Conversation

avandecreme commented May 29, 2015

avandecreme commented May 29, 2015

iangilman Jun 1, 2015

Choose a reason for hiding this comment

iangilman commented Jun 1, 2015

avandecreme commented Jun 1, 2015

iangilman commented Jun 2, 2015

avandecreme commented Jun 2, 2015

iangilman Jun 3, 2015

Choose a reason for hiding this comment

iangilman commented Jun 3, 2015

avandecreme commented Jun 8, 2015

iangilman Jun 10, 2015

Choose a reason for hiding this comment

iangilman commented Jun 10, 2015

avandecreme Jun 11, 2015

Choose a reason for hiding this comment

iangilman Jun 11, 2015

Choose a reason for hiding this comment

iangilman commented Jun 12, 2015

avandecreme commented Jun 12, 2015

iangilman commented Jun 12, 2015

avandecreme commented Jun 13, 2015

iangilman commented Jun 15, 2015