The Trunk: Graphics-nice.296.mcz
Locked
 
|
Nicolas Cellier uploaded a new version of Graphics to project The Trunk:
http://source.squeak.org/trunk/Graphics-nice.296.mcz ==================== Summary ==================== Name: Graphics-nice.296 Author: nice Time: 22 July 2014, 1:28:48.356 am UUID: 3f6d869e-9929-4e08-b137-aae22dd3eee6 Ancestors: Graphics-nice.295 Don't try to accelerate GIF reading by copying the stream in memory. This should be the purpose of a decent file buffering policy, and if ever that was not the case, then we should fix the file buffering policy rather than trying to patch each and every client site. Especially when the patch is testing that the stream is in memory with a class identity to ReadWriteStream!!! Frankly, how do we know that it's not a RWBinaryOrTextStream ;) So why reading an image with a ReadWriteStream? Oh, sure, this is in ImageReadWriter, so we may need a ReadWriteStream? No, no, and no! This thing will either read or write but NEVER read and write. One simple role, one simple class, no UberPotentSwissKnifeStream with UberComplexStateToManage (readPosition + writePosition + readLimit + writeLimit + readBuffer + writeBuffer + readWriteBufferInteraction). What's coming next? Easy to guess, see what (ImageReadWriter>>on:) does frivously to our stream: reset??? Hey, ImageReadWriter, why do you take the responsibility to reset the stream? Did you ask the permission? Did you think of possible side effects? I'm quite sure those reset has something to do with following usage: (ReadWriteStream with: 'foo') next -> nil but: (ReadWriteStream with: 'foo') reset; next -> $f Pure Read/Write madness. There is more room for cleanup here, but only after I get rid of more R/WThing (TM), otherwise I could break some insane expectations. =============== Diff against Graphics-nice.295 =============== Item was changed: ----- Method: AnimatedGIFReadWriter>>allImages (in category 'accessing') ----- allImages | body colorTable | - stream class == ReadWriteStream ifFalse: [ - stream binary. - self on: (ReadWriteStream with: (stream contentsOfEntireFile))]. localColorTable := nil. forms := OrderedCollection new. delays := OrderedCollection new. comments := OrderedCollection new. self readHeader. [(body := self readBody) == nil] whileFalse: [colorTable := localColorTable ifNil: [colorPalette]. transparentIndex ifNotNil: [transparentIndex + 1 > colorTable size ifTrue: [colorTable := colorTable forceTo: transparentIndex + 1 paddingWith: Color white]. colorTable at: transparentIndex + 1 put: Color transparent]. body colors: colorTable. forms add: body. delays add: delay]. ^ forms! Item was changed: ----- Method: GIFReadWriter>>nextImage (in category 'accessing') ----- nextImage + "Read in the next GIF image from the stream." - "Read in the next GIF image from the stream. Read it all into - memory first for speed." | f thisImageColorTable | - stream class == ReadWriteStream ifFalse: [ - stream binary. - self on: (ReadWriteStream with: (stream contentsOfEntireFile))]. localColorTable := nil. self readHeader. f := self readBody. self close. f == nil ifTrue: [^ self error: 'corrupt GIF file']. thisImageColorTable := localColorTable ifNil: [colorPalette]. transparentIndex ifNotNil: [ transparentIndex + 1 > thisImageColorTable size ifTrue: [ thisImageColorTable := thisImageColorTable forceTo: transparentIndex + 1 paddingWith: Color white ]. thisImageColorTable at: transparentIndex + 1 put: Color transparent ]. f colors: thisImageColorTable. ^ f ! |
Re: The Trunk: Graphics-nice.296.mcz
|
|
2014-07-22 1:29 GMT+02:00 <[hidden email]>: Nicolas Cellier uploaded a new version of Graphics to project The Trunk: the answer is summarized there: readNativeResourceFrom: byteStream | img aStream | (byteStream isKindOf: FileStream) ifTrue:[ "Ugly, but ImageReadWriter will send #reset which is implemented as #reopen and we may not be able to do so." aStream := RWBinaryOrTextStream with: byteStream contents. ] ifFalse:[ aStream := byteStream. ]. Ugly says it all. I'm quite sure those reset has something to do with following usage: |
Re: The Trunk: Graphics-nice.296.mcz
|
|
Why not dispatch to byteStream then? |
Re: The Trunk: Graphics-nice.296.mcz
|
|
2014-07-23 21:42 GMT+02:00 Chris Muller <[hidden email]>:
Because there is a better solution: remove the undue reset from ImageReadWriter>>on: There is no reason why it should reset the stream, that's not its business, and it won't work with some Stream species. This reset is just a hack necessary when you use a ReadWriteStream for reading... Since people tend to forget it, the reset has been placed at ReadWriteStream consumption site. Maybe a better place for reset would be when creating a ReadWriteStream for reading, but creation message are direction agnostic (do not distinguish reading from writing usage)... But if we create a ReadWriteStream for reading and exclusively for reading - then maybe we'd better just create a ReadStream... That's all the spirit of my changes. ReadWriteStream carries complexity, it has both state for reading and for writing with some unobvious interactions, and requires some hacking like sending #reset and other niceties. But 99% of time we are not going to need this complexity, because in most use cases we will read XOR write, in some cases write THEN read (ReadWriteStream is just there to avoid a copy), but very very rarely interleave read and write. If we just use a ReadStream when we want to read and a WriteStream when we want to write, we better express our intentions, and we simplify code. No use for strange state hacking (reset), not use for ugly class testing (isKindOf:), or its beautified dispatch form... By the way, how would we name the selector? I have a very speaking proposition: byteStream robustifyForThoseClientExpectingAReadWriteStreamAndHackingStreamStateWithAReset. Nicolas |
Re: The Trunk: Graphics-nice.296.mcz
|
|
>>> 2014-07-22 1:29 GMT+02:00 <[hidden email]>:
>>> >>>> Nicolas Cellier uploaded a new version of Graphics to project The Trunk: >>>> http://source.squeak.org/trunk/Graphics-nice.296.mcz >>>> >>>> ==================== Summary ==================== >>>> >>>> Name: Graphics-nice.296 >>>> Author: nice >>>> Time: 22 July 2014, 1:28:48.356 am >>>> UUID: 3f6d869e-9929-4e08-b137-aae22dd3eee6 >>>> Ancestors: Graphics-nice.295 >>>> >>>> Don't try to accelerate GIF reading by copying the stream in memory. >>>> This should be the purpose of a decent file buffering policy, and if >>>> ever that was not the case, then we should fix the file buffering policy >>>> rather than trying to patch each and every client site. >>>> >>>> Especially when the patch is testing that the stream is in memory with a >>>> class identity to ReadWriteStream!!! >>>> Frankly, how do we know that it's not a RWBinaryOrTextStream ;) >>>> >>>> So why reading an image with a ReadWriteStream? >>>> Oh, sure, this is in ImageReadWriter, so we may need a ReadWriteStream? >>>> No, no, and no! >>>> This thing will either read or write but NEVER read and write. >>>> One simple role, one simple class, no UberPotentSwissKnifeStream with >>>> UberComplexStateToManage (readPosition + writePosition + readLimit + >>>> writeLimit + readBuffer + writeBuffer + readWriteBufferInteraction). >>>> >>>> What's coming next? >>>> Easy to guess, see what (ImageReadWriter>>on:) does frivously to our >>>> stream: >>>> reset??? Hey, ImageReadWriter, why do you take the responsibility to >>>> reset the stream? Did you ask the permission? Did you think of possible side >>>> effects? >>> >>> >>> the answer is summarized there: >>> >>> readNativeResourceFrom: byteStream >>> | img aStream | >>> (byteStream isKindOf: FileStream) ifTrue:[ >>> "Ugly, but ImageReadWriter will send #reset which is implemented >>> as #reopen and we may not be able to do so." >>> aStream := RWBinaryOrTextStream with: byteStream contents. >>> ] ifFalse:[ >>> aStream := byteStream. >>> ]. >>> >>> Ugly says it all. >> >> >> Why not dispatch to byteStream then? >> >> > Because there is a better solution: remove the undue reset from > ImageReadWriter>>on: > There is no reason why it should reset the stream, that's not its business, > and it won't work with some Stream species. > This reset is just a hack necessary when you use a ReadWriteStream for > reading... > Since people tend to forget it, the reset has been placed at ReadWriteStream > consumption site. > Maybe a better place for reset would be when creating a ReadWriteStream for > reading, but creation message are direction agnostic (do not distinguish > reading from writing usage)... > But if we create a ReadWriteStream for reading and exclusively for reading - > then maybe we'd better just create a ReadStream... > That's all the spirit of my changes. Okay, I thought you wrote that method and hated it because you were "forced" to use "isKindOf: FileStream". Hence my suggestion that could easily be cleaned with a dispatch, but now I see you're commenting about a much broader ugliness; one in the design and/or usage of ReadWriteStream. Okay. > ReadWriteStream carries complexity, it has both state for reading and for > writing with some unobvious interactions, and requires some hacking like > sending #reset and other niceties. > But 99% of time we are not going to need this complexity, because in most > use cases we will read XOR write, in some cases write THEN read > (ReadWriteStream is just there to avoid a copy), but very very rarely > interleave read and write. Well, I don't know about the 99%, but as long as you acknowledge that there ARE use-cases needing interleaved reads and writes. _Magma_ for example! :) Another is the .changes file, right? > If we just use a ReadStream when we want to read and a WriteStream when we > want to write, we better express our intentions, and we simplify code. Yes, when there is ONLY a need to read, a ReadStream is appropriate, a ReadWriteStream, not. When there is a need for reading and writing, a ReadWriteStream is appropriate. > No use for strange state hacking (reset), not use for ugly class testing > (isKindOf:), or its beautified dispatch form... > By the way, how would we name the selector? > I have a very speaking proposition: byteStream > robustifyForThoseClientExpectingAReadWriteStreamAndHackingStreamStateWithAReset. > > Nicolas > |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |