Skip to content

Add tiled iterators to ImageBuf#393

Closed
btdavis wants to merge 8 commits intoAcademySoftwareFoundation:masterfrom
btdavis:ibiter
Closed

Add tiled iterators to ImageBuf#393
btdavis wants to merge 8 commits intoAcademySoftwareFoundation:masterfrom
btdavis:ibiter

Conversation

@btdavis
Copy link

@btdavis btdavis commented Jul 5, 2012

The changes are much easier to follow in the individual commits.

  • Add an iterator base class: contains information about iteration range, image bounds, position, and related functions
  • Iterator and ConstIterator inherit from IteratorBase; derived classes contain a reference to imagebuf, data proxy, access operations, and operator++
  • Add a TiledIterator and TiledConstIterator, which minimize calls to ImageBuf::retile during normal iteration
  • Use tiled iterators in ImageBuf::get_pixels and IvImage::copy_pixel_channels

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an advantage to having this return a 'const int &' versus just an 'int'?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is left over from testing. I tried to see if reducing function calls would help.
Something like this:

const int &x = iter.x();
const int &y = iter.y();

for (iter, begin, end)
    //use refs instead of x(), y()

But the compiler was already inlining and optimizing x() and y() such that it made no real difference.

@lgritz
Copy link
Collaborator

lgritz commented Jul 5, 2012

I really like the refactoring of common Iterator/ConstIterator functionality into IteratorBase.

I need to read over TiledIterator more carefully, that's where most of the juicy stuff is.

Have you seen performance improvement from using TiledIterators? You may only see it for large images, or when operating on images backed by an ImageCache that is being heavily used (and thus making tile access more coherent is likely to be a big advantage).

@lgritz
Copy link
Collaborator

lgritz commented Aug 6, 2012

I have not forgotten about you!

I'm in the process of squashing and merging this.

Will update shortly.

@lgritz
Copy link
Collaborator

lgritz commented Jan 16, 2013

I'm going to close this pull request. The iterators have been overhauled in the mean time, so this would be an extremely tricky merge. Additionally, I've investigated the timing issues in great detail, and the difference in iterator performance between the usual scanline order iteration and exhausting each tile in turn is minuscule -- not worth complicating the implementation at this time.

@lgritz lgritz closed this Jan 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants