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

Subject: Re: [boost] [gil::io] Feedback for scanline_read_iterator
From: Michael Marcin (mike.marcin_at_[hidden])
Date: 2013-02-22 12:59:58

Christian Henning <chhenning <at> gmail.com> writes:
> 
> >
> > 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.
> 

I think this might be even worse unfortunately, if the iterator is copied, such 
as with (*i++) it's going to make a full copy of the buffer. Now that readers 
have a begin and end maybe the reader could own the buffer?

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

Sure that makes sense, but the interface lies IMO. If I have jpeg and I call 
skip( buffer, 0 ) it's not going to be skipping over scanline 0 (unless it 
happens to be the first call to read/skip), it's just skipping forward one 
scanline, the second parameter is unused.

Similarly the TIFF reader seems to read skipped scanlines unnecessarily as skip 
forwards to read but the TIFF interface supports reading any scanline you want 
directly with TIFFReadScanline.

> >
> > 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.
> 

iterator_facade is super duper simple. You just implement a handful of private 
member functions, specify the appropriate iterator tag and it generates the rest 
of the iterator for you with no hassles. I highly recommend trying it.

> 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?
> 

I'm not confident your increment operator is correct. I don't think a correct 
increment can call skip/read for an input_iterator and yours can still call 
skip.

I think this is a better implementation (recreated here with more comments and 
without the iterator_facade so as not to confuse anything)

scanline_read_iterator( Reader& reader )
: _pos( 0 )
, _last_pos( -1 ) 
{
}
        
scanline_read_iterator< Reader >& operator++()
{
    // simply advance _pos, that's all we need to do
    ++_pos;
    return (*this);
}

reference operator*()
{
    // if this is the first dereference, or we've advanced 
    // pos since the last dereference we have work to do
    // otherwise buffer already contains the scanline data for _pos
    if ( _last_pos != _pos )
    {
        // advance last_pos skipping any lines until we reach _pos
        // if we reach _pos after one increment we don't need to do
        // any skipping and we can just read as it's the next scanline
        while ( ++_last_pos != _pos )
        {
            _reader->skip( _buffer, _last_pos );
        }
        _reader->read( _buffer, _pos );
    }
    return _buffer;
}