<Sound Dev> [9] Review Request: 8013586 and 8130305

[Florian Bomers]

Hi,
looks good to me. The new unit test might be a little more verbose
that it would throw an exception in the last Files.delete() statement.
Still, as said, this kind of refactoring is error prone.

Always resetting the stream after reading the file format does not
contradict the specification. The specification says that it may need
to mark and reset the stream. It is up to the implementation if and
when any of that is actually done.

Because there is no functional difference and to not accidentally
introduce a regression with applications relying on the current
behavior, I'd keep it as it is. There is a certain consistency in it
when getAudioFileFormat always resets the stream.

WaveExtensibleFileReader -- I assume it got imported with gervill.
Such Wave files are rather rare.

Thanks,
Florian



On 21.07.2015 17:17, Sergey Bylokhov wrote:
> Hello.
> Please review the fix for jdk9. Two issues were fixed:
> 8013586: audioInputStream.close() does not release the resource
> 8130305: AudioSystem behavior depends on order that providers are located
> 
> Description:
>  - Streams were not closed when necessary(ex: WaveFloatFileReader.
> getAudioInputStream(File)). In the fix they are closed always in case
> of any exceptions.
>  - Some of the readers do not reset the streams when they throw an
> UnsupportedAudioFileException exception.
>  - Some of the readers(like AiffFileReader) do not wrap
> (FileInputStream, etc) in BufferedInputStream.
> 
> In the fix I merge all implementations of getAudioXXX to the
> SunFileReader.
> 
> Note that I added a comment to the
> SunFileReader.getAudioFileFormat(InputStream) that implementation
> contradicts the specification, and it seems the javadoc should be
> updated? And I created a new issue about EOFException:
> https://bugs.openjdk.java.net/browse/JDK-8131974
> 
> Also I have a related question about WaveExtensibleFileReader why it
> was not added to the spi.AudioFileReader? Because of this
> WaveExtensibleFileReader actually is never used. Is that intentionally?
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8013586
> Webrev can be found at: http://cr.openjdk.java.net/~serb/8013586/webrev.02
> 
> -- 
> Best regards, Sergey. 
> 

-- 
Florian Bomers
Bome Software

everything sounds.
http://www.bome.com
__________________________________________________________________
Bome Software GmbH & Co KG        Gesellschafterin:
Dachauer Str.187                  Bome Komplementär GmbH
80637 München, Germany            Geschäftsführung: Florian Bömers
Amtsgericht München HRA95502      Amtsgericht München HRB185574