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

[Phil Race]

On 07/21/2015 08:17 AM, 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.

Is this the issue behind the somewhat cryptic "8130305: AudioSystem 
behavior depends on order that providers are located" ?

>  - Some of the readers(like AiffFileReader) do not wrap 
> (FileInputStream, etc) in BufferedInputStream.

  80     public AudioInputStream getAudioInputStream(final InputStream stream)
   81             throws UnsupportedAudioFileException, IOException {
   82         stream.mark(200);
   83         try {
   84             final AudioFileFormat fileFormat = getAudioFileFormatImpl(stream);

You aren't wrapping the stream here. This is OK if it is called via one 
of the other
two methods that do, but can it not be called directly ?

Same for

public final AudioFileFormat getAudioFileFormat(final InputStream stream)

but it may not matter on the methods that just read a few bytes of header anyway ..

stream.mark(200);

seems arbitrary and I assume it was your calculation of the max likely to
be needed by any subclass but it would be good to add a comment about this.


-phil.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/sound-dev/attachments/20150721/3c59f5e0/attachment.html>