Fixed issue #87 so that now SRT subtitles are read live and made local and remote file access consistent and easily extendable.

[enternoescape]

This does not address multi-part recordings that have a subtitle file for each segment.

[Narflex]

My personal preference is to never import .. I've been bitten by that before where it was using the wrong class because I wasn't aware something with the same class name was in a . imported package.

[enternoescape]

I can change that back, I left cleanup checked when I committed and it consolidated those automatically. I've noticed your preference to not import.

[Narflex]

OK, it changed my comment when I put a "dot star" and apparently inferred that to mean italics...odd. I'm totally fine with imports, its just the wildcard ones I'm not a fan of.

[enternoescape]

The comments are in markdown, so you need to escape certain characters. * It bugs me too sometimes.

I do strive to be a good contributor to this open source project and I have tried to emulate some of the style I've seen even when it's not my preference. I will fix the imports.

[Narflex]

Can you provide a summary of the new file/IO classes you've created? This is a huge change, so it's going to take me some time to go over all of it and any additional information (aside from what we've already talked about in other comments) will help speed that up. :) It'll also help anybody else trying to understand how to use all of this.

[enternoescape]

These classes all use IO:
sage.io.SageFileSource - An implementation used by the classes sage.io.LocalSageFile, sage.io.RemoteSageFile, sage.io.BufferedSageFile and sage.io.EncryptedSageFile. It provides a standardized framework for IO operations in SageTV. The idea is that each one tries to focus on doing one thing well so if for example we come up with a 3rd way we want to read a file/data or maybe even an ISO, you only need to write exactly that part and don't need to think about things like buffering or adding methods to get a single byte, short int, long. They are designed to be stackable with an actual data source at the bottom.

sage.io.LocalSageFile - This is 95% RandomAccessFile.

sage.io.RemoteSageFile - This uses the media server to access files.

sage.io.BufferedSageFile - This adds a read and write buffer layer on top of any class that implements sage.io.SageFileSource. It does 100% buffered writing and it tries to determine based on the sizes of reads if it needs to buffer fully, some combination or fully read directly from it's source.

sage.io.EncryptedSageFile - This adds an encryption and decryption layer on top of any class that implements sage.io.SageFileSource. This is the same encryption as the sage.FastRandomAccess file class.

These classes add other data processing capabilities over top of the sage.io.SageFileSource implementations:
sage.io.SageDataFile - Adds java.io.DataInput and java.io.DataOutput implementations over top of any sage.io.SageFileSource implementation.

sage.io.SageInputStream - Converts any sage.io.SageFileSource into an java.io.InputStream.

sage.io.SageOutputStream - Converts any sage.io.SageFileSource into an java.io.OutputStream.

sage.io.SageIOFileChannel - Implements FileChannel over any sage.io.SageFileSource. This one is dependent on sage.io.BufferedSageFile. If is it not present at the top level, it will automatically wrap it around the provided class, but it will then always flush the read or write buffers after every write to ensure you get the expected behavior. This of course negates any performance benefits. It needs the buffer layer since it already has byte arrays allocated to transfer between ByteBuffer and byte[].

These classes all use NIO:
sage.nio.SageFileChannel - This interface implements most of what you would expect to see in a FileChannel, plus it allows you to change out the source file which was needed for the transitioning between files in sage.media.bluray.BlurayFile. This interface is meant to be stackable similar to sage.io.SageFileSource. It has the methods transferTo(long, WritableByteChannel) and transferFrom(ReadableByteChannel, long) which transfer data relative to the current position in the file. tryLock(), lock(), map() and their associated methods are non-functional partly because we don't have any great use for them in SageTV; this is true for all of the classes extending FileChannel that I wrote.

sage.nio.LocalFileChannel - This is nearly a direct implementation of FileChannel and also extends FileChannel with some extra methods per the sage.nio.SageFileChannel interface.

sage.nio.RemoteFileChannel - This accesses files via the media server. It also extends FileChannel with some extra methods per the sage.nio.SageFileChannel interface.

sage.nio.BufferedFileChannel - This wraps around any sage.nio.SageFileChannel and provides both read and write buffer. Depending on how assumed large the transfer per the read or write method determines just how much read or write buffering there really is. For example, the scatter and gather methods will use what's left in the read/write buffer, then read directly since it's expected that you must be doing very large reads to need to use those methods.

sage.media.bluray.BlurayFile - This replaces sage.media.bluray.BlurayNetworkFile and sage.media.bluray.BlurayRandomFile and implements BlurayStreamer. It also extends FileChannel, though that was probably a little much in that case. We now only have one class to maintain for the same functionality. I left all of the old classes because these are somewhat big changes. It wraps sage.nio.BufferedFileChannel around sage.nio.LocalFileChannel or sage.nio.RemoteFileChannel depending on if we have local access or not.

[Narflex]

Can you increment the version number? This is a significant enough change with wide reach that I think it warrants a new microversion increment. :)

[Narflex]

I don't think you should have the isActiveFile() method in here. That's a much higher level concept, and it's also not actually implemented except for network files so I don't want people thinking they can use it. There is MMC.isRecording(File) which will give you the correct answer...BUT that does not work properly unless you are in the server process (i.e. there's no way to call this on SageTVClient, but yes, the client should be accessing files remotely, but for the service case there may be optimizations where it is not).

As I also don't see any actual usages of this yet in the code, removing it doesn't seem like a problem.

[enternoescape]

This makes sense. I removed it.

[Narflex]

I'm curious why you chose to extend RandomAccessFile rather than make it a member inside of here? IIRC, the reason I didn't extend it was because I didn't want to end up using methods that I didn't expect to be used. I notice in yours you make some methods not supported, and so generally you're changing this to really not be like java.io.RandomAccessFile anymore (which is fine); but in that case you probably shouldn't make it extend it.

This class is also more about general byte access into a local file...and RandomAccessFile exposes all those other DataInput/DataOutput methods that it also seems to go beyond the scope of what LocalSageFile was intended to cover.

[enternoescape]

I didn't extend it at first, but then I did in what could be a misguided attempt to have one less method between the top class and the actual data source. I can see how someone might accidentally use that incorrectly. I'll make it less exposed.

[Narflex]

I was curious what you were going to do with the 'readonly' field here. :) When you open a remote file with the media server...you can open it either in read mode or write mode; but not both at the same time (although the MediaServer could be extended to support such a case easily by opening a RandomAccessFile and getting the channel from there rather than a FileOuptutStream object in the openWriteFile method).

The way this is currently done; any calls to the write method in here will fail because the files are being opened in read mode on the server always. Since you've got all the other work in here...I'd say just change MediaServer to use a RandomAccessFile's channel instead of FileOutputStream (the uploadStream field); then make this use OPENWRITEW instead of OPENW if readonly is set to false (and prior to that, request the upload key from the server so it'll grant you write access). That should have no performance implications.

[enternoescape]

I believe I mis-followed what you were doing to get the FileChannel in the media server. There's also a FileChannel.open() static method which is what I would probably use since that will result in just getting a file channel without creating any other classes we aren't actually going to use.

[Narflex]

If you change that behavior; just make sure you're opening it in the proper read/write mode since that's how read/write security is enforced in MediaServer.

[enternoescape]

I removed FileOutputStream and replaced it with FileChannel.open(currFile.toPath(), StandardOpenOption.CREATE, StandardOpenOption.READ, StandardOpenOption.WRITE)

[Narflex]

You should probably optimize it to avoid calling length() unless required. You have this check in a few places, so you may just want to make a private method that does it to prevent code duplication (i.e. isLengthGreater(long))

[enternoescape]

Noted and addressed.

[Narflex]

You can optimize this so that if the first time you send SIZE, you get back the same value for both, you never need to call the remote command again. Once a file goes non-active...it will never go active again.

[enternoescape]

That's only partly true. When the SRT file is growing, the media server as far as I've been able to tell provide any solid indication that is in fact happening.

[Narflex]

Ohhh...right, you're looking at files generated by some other process...not SageTV, so it has no knowledge of whether they are growing or not. Hmmm...that makes it more tricky...and that problem was the main motivation for this work. :) You can put it back the way it was then...especially considering the current code didn't have the optimization I suggested either.

[enternoescape]

I did what I think is a good compromise. I added another constructor that enables you to force it to still check.

[enternoescape]

It calls read(byte[],int,int) and it was returning 0 which I fixed to return -1 which is what should be happening per the contract. I still added an initialization parameter to force the file to be thought active so that SRT subtitles can be read live.

[Narflex]

I wouldn't say this is an exception...of course there's no need for it in the current code...but maybe somebody wants to double encrypt in the future for some reason. :) I'm fine with it either way though.

[Narflex]

Ahhh...OK, but this could then break getting the unencrypted source back properly, so leave it in here...BUT, you should probably loop all the way down the chain to make sure another EncryptedSageFile isn't hiding somewhere else in there. (i.e. keep getting the source of the source until it is null or you find another encrypted one and throw an exception).

[enternoescape]

I suppose the way this is built that's a real possibility that one could be buried in the stack. This is here to help out SageDataFile so it doesn't need to be aware of multiple encryption layers. Either way, I made the search recursive per your suggestion.

[Narflex]

Can you add a finally{} clause which released the media server access so the HashSet on the server doesn't grow without bound?

[enternoescape]

That makes sense.

[Narflex]

OK, I've now read through everything and you can see the remaining comments I had. Great work overall! I didn't go through the updated SRTSubtitleHandler with a fine-toothed comb, but I'm sure you've been using it extensively so it's likely working fine.

And I remembered another good reason to increment the version...then it'll back up the Wiz.bin file when people update to this version...and since you've changed the file handling code for that, we definitely want people to have a backup of it. :)

[enternoescape]

Backups are always nice.

Just an FYI. I will probably not be active for tomorrow and the weekend, so if you're waiting for input whereby silence means acceptance, please wait for Monday before you commit. I'll be essentially in the wilderness which is probably a good thing for me. I spend far to much time coding and browsing the internet. :)

[Narflex]

Perfect. I have family in for the weekend so I wasn't going to get around
to anything until probably Tuesday. :-)

Jeff Kardatzke
Sent from my Android

On Jul 21, 2016 8:37 PM, "Joseph Shuttlesworth" notifications@github.com
wrote:

Backups are always nice.

Just an FYI. I will probably not be active for tomorrow and the weekend,
so if you're waiting for input whereby silence means acceptance, please
wait for Monday before you commit. I'll be essentially in the wilderness
which is probably a good thing for me. I spend far to much time coding and
browsing the internet.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#156 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ANEIDPvSIgGzLRQRXBcnrmlNLJ-ndkLrks5qYDregaJpZM4JPOwa
.

[enternoescape]

My thing is very similar, but it's more like a very large cabin with all of my extended family in the woods.

[Narflex]

The second part of the conditional is inverted over what it originally was.

[enternoescape]

Not sure why I did that. Fixed it.

[Narflex]

OK, had one error I found and replied to another comment. It's almost ready for merging. :)

[Narflex]

One other thing I noticed is that the test code is in /src/test/java, which is somewhat confusing because it has that 'src' root directory...but the other source code is not in that directory. What do you think about changing that to just be 'test/java' instead?

[enternoescape]

That's fine. I originally did test/java, but it felt kind of wrong. I'll change it back to that.

[Narflex]

Woo-hoo! Congrats! It's merged!