Bugs found in library

[jonjamz]

Hi,

I have done some testing on this library and found several bugs that I have started trying to fix. I figured I would share for the benefit of anyone using or relying on this library, and in case someone could provide any help or clarification.

One thing I noticed right away, which I would consider more an unfinished feature than a bug, is that the size limit parameter is never handled anywhere. I simply commented out any calculations relating to this because I don't need it.

Next, it seems that the asUInt32BE String prototype extension may not actually be converting to unsigned integers. I say this because I recently tested one file that reported to have a negative atom length. When I modified readBlob to use an ArrayBuffer instead of a binary string and passed that to a DataView and used getUint32(0) I was presented with a different, non-negative value that seemed to be offset as an unsigned integer would be compared to a signed one.

I have started trying to figure out how to get the library to make use of ArrayBuffers instead of binary strings, as that is now the recommended course (readAsBinaryString has been deprecated and it is widely recommended to use readAsArrayBuffer in its place--see here). However, I would not call this an area of expertise for me yet, so any help here would be greatly appreciated. I have and continue to do a good amount of research around this and am starting to piece everything together slowly, but it seems that nearly every step in this library relies on the existence of the binary string returned from readAsBinaryString.

Finally, and I think this is a big one as well, some of the logic and math in copyChunks seems to be off. I noticed when I first started testing the library that it would never proceed past the third onChunkReady call in any given file, and I seem to have figured this out, although I could use some validation here (in my changes I namespaced the readBlob method as qtfs_readBlob and have done what I can to clarify and comment the code and my changes):

  /*
    Copy over the remaining atoms in chunks, skipping the ftyp and moov
    atoms as they are already processed and written.
  */
  function copyChunks(file, index, totalChunks, limit) {
    var chunks = 3;

    // Start at the beginning of the index.
    var x = 0;
    var atom = index[x];

    // Find the next atom in the index (not ftyp or moov) and set us there.
    function setNextAtom() {
      QtFastStart.onLog(file, 'Setting next non-ftyp/moov atom in `copyChunks`.');
      // Only increment within the bounds of the index.
      if (x + 1 < index.length) {
        x += 1;
        atom = index[x];
      }
      console.log('[QtFastStart] Current atom:', atom, 'index:', index, x);
      // If the current atom is `ftyp` or `moov`, continue until the next
      // one we find that isn't.
      if (atom.type === 'ftyp' || atom.type === 'moov') {
        console.log('[QtFastStart] Current atom is ftyp/moov! Setting next atom...')
        setNextAtom();
      }
    }

    // Callback to run at the end of each chunk.
    function _callback(file, start, end, data) {
      QtFastStart.onLog(file, "Completed reading a chunk in `copyChunks`");
      QtFastStart.onChunkReady(file, chunks, totalChunks, data);
      chunks += 1;

      QtFastStart.onLog(file, `Are we at the end of this atom? End: ${end}, current atom end: ${atom.pos + atom.len - 1}, chunks left? ${chunks <= totalChunks}`);
      // Are we at the end of this atom and is there more data to copy?
      // Let's go to the next atom.
      if (end == atom.pos + atom.len - 1 && chunks <= totalChunks) {
        setNextAtom();
      }

      QtFastStart.onLog(file, `Are there more atoms in the index to evaluate for copying? ${x < index.length}`);

      // As long as we're not at the end of the index...
      if (x < index.length) {
        // Set us to the position immediately after the last ending position.
        var pos = end + 1;
        // Get ending position of the current atom.
        var currentAtomEnd = atom.pos + atom.len - 1;
        // As long as we're not at the end of the current atom...
        QtFastStart.onLog(file, `Are we at the end of the current atom? ${!pos < currentAtomEnd}`);
        if (pos < currentAtomEnd) {
          // Copy next chunk:

          // Calculate end byte for this chunk.
          var chunkEndByte = (
            // NOTE: I think this is the error! We're trying to see if we
            // should get from current pos to the end of this atom, or
            // go until current pos + chunk size (which would have to be
            // less than from current pos to end of atom).
            //
            // Looks like we are calculating size from current pos to
            // end of atom incorrectly: `atom.len - pos` when `pos` is
            // in the context of the entire file, not the current atom.
            //
            // We need to get `currentAtomEnd - pos` instead.

            // Is the distance from here to the end of the atom less than
            // the chunk size? If so, end this at the end of the current
            // atom. Otherwise, end this at the chunk size.
            (currentAtomEnd - pos < QtFastStart.chunkSize) ?
            currentAtomEnd : (pos + QtFastStart.chunkSize)
          );

          file.qtfs_readBlob(pos, chunkEndByte, _callback, QtFastStart.onError);
        }
      }
    }

    // START
    if (atom.type === 'ftyp' || atom.type === 'moov') {
      setNextAtom();
    }

    // Limit this read to the desired chunk size.
    // --
    // If atom size is larger than the chunk size (in bytes) we use the
    // chunk size, otherwise we use the atom size for the next chunk.
    var chunkEndByte = atom.pos + ((atom.len > QtFastStart.chunkSize) ?
      QtFastStart.chunkSize : atom.len) - 1;

    QtFastStart.onLog(file, `Copying from ${atom.pos} to ${chunkEndByte}`);

    file.qtfs_readBlob(atom.pos, chunkEndByte, _callback, QtFastStart.onError);
  }

Here is the original copyChunks code for reference:

    /*
        Copy over the remaining atoms in chunks, skipping the ftyp and moov
        atoms as they are already processed and written.
    */
    function copyChunks(file, index, totalChunks, limit) {
        var chunks = 3;

        var x = 0;
        var atom = index[x];

        function _callback(file, start, end, data) {
            QtFastStart.onChunkReady(file, chunks, totalChunks, data);
            chunks += 1;

            if (end == atom.pos + atom.len - 1 && chunks <= totalChunks) {
                // Get the next atom to copy!
                do {
                    x += 1;
                    atom = index[x];
                } while (x < index.length && (atom.type in {"ftyp":'', "moov":''}))
            }

            if (x < index.length) {
                var pos = end + 1;
                if (pos < atom.pos + atom.len) {
                    // Copy next chunk
                    file.readBlob(pos, ((atom.len - pos < QtFastStart.chunkSize) ? (pos + atom.len) : (pos + QtFastStart.chunkSize)), _callback);
                }
            }
        }

        while (x < index.length && (atom.type in {"ftyp":'', "moov":''})) {
            x += 1;
            atom = index[x];
        }

        file.readBlob(atom.pos, atom.pos + ((atom.len > QtFastStart.chunkSize) ? QtFastStart.chunkSize : atom.len), _callback);
    }

I think that the idea of porting the original library to JS for use in the browser is brilliant and incredibly useful, and I am really hoping to get it working properly. Looking forward to any feedback or recommendations.

Thanks!

[randy-girard]

Have you made any more progress? I would be interested in work on this as well. I have been trying to use this library, but it seems to be messing up my files a little bit. They don't quite play right after going through this tool

[danielyaa5]

hello I know its been a while but was wondering if you could share your updates to this library @randy-girard @jonjamz

[jonjamz]

@danielyaa5 I couldn't get this to work the way I wanted and eventually moved on. If you figure it out, please share!

Thanks,

Jon