Recent

Author Topic: Bug in TReadBufStream and proposed fix  (Read 18039 times)

glorfin

  • Full Member
  • ***
  • Posts: 148
  • LMath supporter
Bug in TReadBufStream and proposed fix
« on: August 01, 2017, 04:44:05 pm »
I found that TReadBufStream.Seek function did not work properly, especially when seeking backwards.
Indeed, here is its code:
Code: Pascal  [Select][+][-]
  1. function TReadBufStream.Seek(const Offset: Int64; Origin: TSeekOrigin): Int64;
  2. begin
  3.   FakeSeekForward(Offset,Origin,FTotalPos);
  4.   Result:=FTotalPos; // Pos updated by fake read
  5. end;
  6.  
First, and important. procedure TStream.FakeSeekForward does not seek backward (as the name suggests).

Second, updated after connet of howardpc:
Third parameter in FakeSeekForward has const modifier, so it should not be changed by the call.
Then what means comment "Position updated by fake read"?

My suggestion is to rewrite Seek function as follows (in my project this worked nicely):

Code: Pascal  [Select][+][-]
  1. function TReadBufStream.Seek(const Offset: Int64; Origin: TSeekOrigin): Int64;
  2. var
  3.   NewTotalPos, Delta, NewBufPos: Int64;
  4. begin
  5.   case Origin of
  6.     soCurrent:
  7.       NewTotalPos := FTotalPos + Offset;
  8.     soBeginning: begin
  9.       if Offset < 0 then
  10.         InvalidSeek;
  11.       NewTotalPos := Offset;
  12.     end;
  13.     soEnd: begin
  14.       if Offset > 0 then
  15.         InvalidSeek;
  16.       NewTotalPos := Size + Offset;
  17.     end;
  18.   end;
  19.   if NewTotalPos > Size then
  20.     NewTotalPos := Size;
  21.   if NewTotalPos < 0 then
  22.     NewTotalPos := 0;
  23.   Delta := NewTotalPos - FTotalPos;
  24.   NewBufPos := Delta + FBufPos;
  25.   if (NewBufPos > 0) and (NewBufPos < FBufSize) then
  26.   begin
  27.     FBufPos := NewBufPos;  //if Seek function moves inside the data which are in the Buffer, we only reposition FBufPos
  28.     FTotalPos := NewTotalPos;
  29.   end
  30.   else // otherwise find desired place in the source stream and invalidate buffer
  31.   begin
  32.     FTotalPos := FSource.Seek(Offset,Origin);
  33.     FBufPos := 0;
  34.     FBufSize := 0;
  35.   end;
  36.   Result := FTotalPos;
  37. end;
  38.  

By the way, what this function must return? I, as mention in comment, return new position relative to beginning. Is it expected?

Best regards to all!
« Last Edit: August 04, 2017, 12:24:54 am by glorfin »

howardpc

  • Hero Member
  • *****
  • Posts: 4144
Re: Bug in TReadBufStream and proposed fix
« Reply #1 on: August 01, 2017, 05:47:29 pm »
Code: Pascal  [Select][+][-]
  1. function TReadBufStream.Seek(const Offset: Int64; Origin: TSeekOrigin): Int64;
  2. begin
  3.   FakeSeekForward(Offset,Origin,FTotalPos);
  4.   Result:=FTotalPos; // Pos updated by fake read
  5. end;
  6.  
... Origin parameter has const modifier, so it should not be changed by the call.

I don't know where this code comes from, but the Seek call does not change Origin, it simply plugs its value into the call to Fake. If it did change its value, and it is indeed a const parameter, the compiler would refuse the construction.

glorfin

  • Full Member
  • ***
  • Posts: 148
  • LMath supporter
Re: Bug in TReadBufStream and proposed fix
« Reply #2 on: August 01, 2017, 06:16:50 pm »
Oops, sorry, in original post I meant third parameter, FTotalPos. It is supposed to be changed by the call (see the comment), but in fact it is not.
But this was side note; important is that you could not move backwards with this seek function. That is why I had to rewrite it.
« Last Edit: August 01, 2017, 06:21:14 pm by glorfin »

Remy Lebeau

  • Hero Member
  • *****
  • Posts: 1312
    • Lebeau Software
Re: Bug in TReadBufStream and proposed fix
« Reply #3 on: August 02, 2017, 01:40:37 am »
By the way, what this function must return? I, as mention in comment, return new position relative to beginning. Is it expected?

Yes, Seek() is meant to return the new absolute position from the beginning of the stream.
Remy Lebeau
Lebeau Software - Owner, Developer
Internet Direct (Indy) - Admin, Developer (Support forum)

glorfin

  • Full Member
  • ***
  • Posts: 148
  • LMath supporter
Re: Bug in TReadBufStream and proposed fix
« Reply #4 on: August 02, 2017, 10:53:47 am »
Yes, Seek() is meant to return the new absolute position from the beginning of the stream.
Thank you!
Now would be nice to incorporate this new code into BufStream, because old one really does not work.

Thaddy

  • Hero Member
  • *****
  • Posts: 14213
  • Probably until I exterminate Putin.
Re: Bug in TReadBufStream and proposed fix
« Reply #5 on: August 02, 2017, 11:01:09 am »
Streams are streams... They are intended to stream in 1 (one) direction. They are not files.... This is lack of knowledge. It is also basically and fundamentally wrong to implement such features as requested on streams: then they behave like files..... Which are not streams..... <sigh and grumpy  >:D >:D >:D>

To explain it for noobs or indifferent amateurs:
A stream is like a flow of water, you can not go back in that flow, it just passed you, you could have caught the fish....
A stream is sequential and by its very nature unidirectional, not bi-directional. Don't try to make it so. In that case it's a file....

I know this sounds strange, since some streams actually allow to set position, but than it is simply a matter of the wrong name for stream. It becomes a file....

Computer science basics...In any language...
« Last Edit: August 02, 2017, 11:08:10 am by Thaddy »
Specialize a type, not a var.

glorfin

  • Full Member
  • ***
  • Posts: 148
  • LMath supporter
Re: Bug in TReadBufStream and proposed fix
« Reply #6 on: August 02, 2017, 11:10:47 am »
It is also basically and fundamentally wrong to implement such features as requested on streams: then they behave like files.....
This feature is implemented long ago for any stream except Bufstream. And I don't see wha we should impose artificial limitations just because of name. We know anyway that TFileStream, for example, is just a convenient superstructure on top of file. And, by the way, when you are looking streaming video over internet, you nevertheless can rewind it.  O:-)  O:-)  O:-)
« Last Edit: August 02, 2017, 11:16:12 am by glorfin »

Thaddy

  • Hero Member
  • *****
  • Posts: 14213
  • Probably until I exterminate Putin.
Re: Bug in TReadBufStream and proposed fix
« Reply #7 on: August 02, 2017, 11:15:26 am »
A bufstream is actually a stream filter..... with sizeof(buffer) as file.
A stream is infinite... a buffer is finite....
All computer science basics.
I know that you can rewind a "stream" but that is a stream filter, not a stream. Amateurs or inadequately educated programmers confuse this. So much so that you demand it? WRONG!@
« Last Edit: August 02, 2017, 11:19:20 am by Thaddy »
Specialize a type, not a var.

glorfin

  • Full Member
  • ***
  • Posts: 148
  • LMath supporter
Re: Bug in TReadBufStream and proposed fix
« Reply #8 on: August 02, 2017, 11:22:30 am »
A bufstream is actually a stream filter..... with sizeof(buffer) as file.
A stream is infinite... a buffer is finite....
All computer science basics

TBufStream is intended simply for buffered file input-output operations. It owns TFileStream, reads first into own buffer and then allows to read small chunks of data from this buffer. Size of Buffer seldom is equal to the size of file.

However, I have a feeling that our discussion easily can become excessively long and fruitless and by no means constructive. So, my suggestion is that we both remain with own point of view.

Blestan

  • Sr. Member
  • ****
  • Posts: 461
Re: Bug in TReadBufStream and proposed fix
« Reply #9 on: August 02, 2017, 11:26:32 am »
streams by concept are not only unidirectional but also dont have size ... its not guaranteed in any circumstance that you can read an exact amount of data ... even more in some environments streams dont even wait for you to read the data ... you must plug in to the stream and start reading if you discover some markers you can read some reasonable data and analize it ... thats all...

seek(), eof bof etc functions are related to files .. even more to databases...
note that in the early days the file system was a superset of the database not like now when the  db is contained ( in most cases ) in a file within the filesystem 
Speak postscript or die!
Translate to pdf and live!

Blestan

  • Sr. Member
  • ****
  • Posts: 461
Re: Bug in TReadBufStream and proposed fix
« Reply #10 on: August 02, 2017, 11:29:29 am »
@glorfin: if tbufstream own a filestream what is the point -> this is a double buffering because the OS already buffers in most scenarios all the open file handles
Speak postscript or die!
Translate to pdf and live!

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11383
  • FPC developer.
Re: Bug in TReadBufStream and proposed fix
« Reply #11 on: August 02, 2017, 11:57:02 am »
Streams are streams... They are intended to stream in 1 (one) direction. They are not files.... This is lack of knowledge. It is also basically and fundamentally wrong to implement such features as requested on streams: then they behave like files..... Which are not streams..... <sigh and grumpy  >:D >:D >:D>


Pure streams are not seekable (forward or backwards. In theory you could emulate forward by reading dummy bytes) But I think you can assume that streams that implement Seek are seekable. :P
 

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11383
  • FPC developer.
Re: Bug in TReadBufStream and proposed fix
« Reply #12 on: August 02, 2017, 11:58:04 am »
@glorfin: if tbufstream own a filestream what is the point -> this is a double buffering because the OS already buffers in most scenarios all the open file handles

No it does not. Using a buffered stream instead of a non buffered one can speed up many small reads/writes.   At least on Windows it does, and the differences can be very pronounced.

Thaddy

  • Hero Member
  • *****
  • Posts: 14213
  • Probably until I exterminate Putin.
Re: Bug in TReadBufStream and proposed fix
« Reply #13 on: August 02, 2017, 12:14:50 pm »
But I think you can assume that streams that implement Seek are seekable. :P
Which means they implement a stream filter. The rest of your explanation is as always correct. Files are seekable, streams are not.
It is just that somebody decided not to document it as such and introduced a misnominer that is hard to get rid of.
In this case a TBufStream is a stream filter, not the actual stream.
« Last Edit: August 02, 2017, 12:18:11 pm by Thaddy »
Specialize a type, not a var.

Blestan

  • Sr. Member
  • ****
  • Posts: 461
Re: Bug in TReadBufStream and proposed fix
« Reply #14 on: August 02, 2017, 12:38:00 pm »
@marcov: msnd :"  File buffering is usually handled by the system behind the scenes and is considered part of file caching within the Windows operating system unless otherwise specified. "
Speak postscript or die!
Translate to pdf and live!

 

TinyPortal © 2005-2018