Re: [boost] [gil::io] Feedback for scanline_read_iterator

Subject: Re: [boost] [gil::io] Feedback for scanline_read_iterator
From: Christian Henning (chhenning_at_[hidden])
Date: 2013-02-21 21:22:11

Hi Michael,

>
> I'm not sure exactly what _reader->clean_up(); does but the destructor seems
> likely wrong. If you make a copy of the iterator and the original or the
> copy gets destructed it calls clean_up while the other copy may still be in
> use. Shouldn't clean_up just be part of the scanline_reader destructor and
> not the iterator?

I have removed clean_up. It was not used by any image format implementation.

>
> The set_reader and set_buffer member functions seem unnecessary and confuse
> things.

Done.

>
> I think the checks at the top of the dereference operator are unnecessary.
> You're reporting programming errors with exceptions.
>
> if( _reader == NULL ) { io_error( "Reader cannot be null for this
> operation." ); }
> if( _buffer == NULL ) { io_error( "Buffer cannot be null for this
> operation." ); }
>
> Same in increase_pos
> if( this->_reader == NULL ) { io_error("Reader cannot be null for this
> operation."); }

Done.

>
> The process of generating the buffer and supplying it to the iterator feels
> strange to me but I could be wrong.

I moved the buffer creation internally.

>
> There is an equal member function that has nothing to do with operator==.
> Seems like equal should just go away.

OK.

>
> I don't see why end has to be constructed in that manner. It would seem to
> me that an end iterator constructed with the reader makes the implementation
> a whole lot simpler. I would prefer an interface where you just call
> reader.begin() and reader.end() to get your iterators. You'll no longer need
> a sentinel value for past-the-end, _pos could be initialized to
> reader->_info._height.

The readers now have a begin() and end() members.

>
> Speaking of which is _info._height part of the Reader concept? Is the Reader
> concept documented anywhere I couldn't find it but I only did a cursory
> search.

No there is no Reader concept. What do you have in mind?

>
> I don't really understand the second parameter to _reader->skip and
> _reader->read. It seems to be ignored for everything except the TIFF reader.
> Which leads me to believe that if TIFF reader needs to keep track of its
> current row it should do so internally as all the other readers track their
> internally (seemingly implicitly). Also I'm not sure but given the interface
> it seems that TIFF skip could maybe be a noop if the row is passed to read
> still or just a simple increment of an internal row counter.

Skipping can be different for different formats. jpeg for instance
still reads the scanline when skipping. For now, I think, we are good.

>
>
> Note also that I think the requirement on InputIterator concept that
> Postincrement and dereference (*i++) be valid forces lazy evaluation. If you
> did eager reading/skipping somehow in increment you'd break that expression.

Yes, the postincrement operator is missing. I'll have to look up how
it's usually implemented.

>
> Here is a sketch of a possible implementation.
> http://codepad.org/9hjn3SpQ
>

I see you're using iterator_facade. I was a little afraid of using it
since I have never used it before.

What do you think of the updated implementation. I think we are
getting closer to what we want! If you have the time, would you mind
and have another look?

Thank you very much for your insight. It's been very helpful!

Regards,
Christian