Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multistream support to BZip2 #417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add multistream support to BZip2 #417

wants to merge 1 commit into from

Conversation

piksel
Copy link
Member

@piksel piksel commented Feb 1, 2020

Fixes #413.
Fixes #162.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@Numpsy
Copy link
Contributor

Numpsy commented Feb 17, 2020

(fixes #413 rather than 276?)

@piksel
Copy link
Member Author

piksel commented Feb 17, 2020

Gah. Github! Instead of using the number I actually typed, it auto-completed to the first search result that started with the number (413).

@Numpsy
Copy link
Contributor

Numpsy commented Feb 19, 2020

Not sure if this would be considered a real issue or not, but in case:

Does the usage of the Length property on the baseStream have any ramifications on the supported types of input stream?

Example: Say that I have a Zip file that contains a BZip2 file. With the current code I appear to be able to open a BZip2InputStream on the stream that is returned from ZipFile.GetInputStream and read the data from it, but with this change I get

System.NotSupportedException : InflaterInputStream Length is not supported

I'm not sure off hand is BZip2InputStream is inteded to support such streams though? (might possibly also happen if you tried it with a network stream or some such as well)

@philippk80
Copy link

In which release this will be available?

@piksel
Copy link
Member Author

piksel commented Mar 26, 2020

This will probably be in the next release. I have been super busy and neglected this unfortunately. Hopefully I will get time this weekend to fix current issues.

/// Construct instance for reading from stream
/// </summary>
/// <param name="stream">Data source</param>
public BZip2InputStream(Stream stream) : this(stream, true) { }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public BZip2InputStream(Stream stream) : this(stream, true) { }
public BZip2InputStream(Stream stream) : this(stream, stream.CanSeek) { }

Comment on lines +297 to +307
long bytesProcessed = baseStream.Position - processedLastStream;
long bytesLeft = baseStream.Length - baseStream.Position;
if (multiStream && bytesProcessed > 0 && bytesLeft > 0)
{
processedLastStream = bytesProcessed;
Initialize();
}
else
{
return -1; // ok
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
long bytesProcessed = baseStream.Position - processedLastStream;
long bytesLeft = baseStream.Length - baseStream.Position;
if (multiStream && bytesProcessed > 0 && bytesLeft > 0)
{
processedLastStream = bytesProcessed;
Initialize();
}
else
{
return -1; // ok
}
if(!multiStream) {
return -1; // no multistream support, so just end it here
}
long bytesProcessed = baseStream.Position - processedLastStream;
long bytesLeft = baseStream.Length - baseStream.Position;
if (bytesProcessed > 0 && bytesLeft > 0)
{
processedLastStream = bytesProcessed;
Initialize();
}
else
{
return -1; // ok
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable

/// <param name="outStream">The output stream to receive the decompressed data.</param>
/// <param name="isStreamOwner">Both streams are closed on completion if true.</param>
public static void Decompress(Stream inStream, Stream outStream, bool isStreamOwner)
=> Decompress(inStream, outStream, isStreamOwner, true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
=> Decompress(inStream, outStream, isStreamOwner, true);
=> Decompress(inStream, outStream, isStreamOwner, inStream.CanSeek);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain if there aren't any streams that support seeking but don't allow access to their length, but maybe that's too much of an unusual situation to worry about, and as long as the functionality can be disabled that should cover the situation if needed.

Copy link
Member Author

@piksel piksel Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if seeking can even be achieved if you don't allow Length (since the SeekOrigin parameter of Seek() call can be set to End.
The Stream documentation basically thinks of them as mutually inclusive:
https://docs.microsoft.com/en-us/dotnet/standard/io/?view=netframework-4.8#streams

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I said too unusual to worry about (it might not make sense, but that doesn't guarantee it won't happen!).
Not a reason to avoid making the change though

@piksel
Copy link
Member Author

piksel commented Mar 29, 2020

@Numpsy I made suggestions that should make it compatible with non-seekable streams. It will not access Stream.Length if multistream is not enabled, and it defaults to enable it only if the input stream supports seeking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants