Comcast / gots

MPEG Transport Stream handling in Go
Other
308 stars 88 forks source link

packet: Sync potentially double buffers #87

Closed kortschak closed 5 years ago

kortschak commented 6 years ago

The Sync function requires that the input reader be a *bufio.Reader. This is to allow confirmation of the following sync byte. In some cases (when the reader is already read into a []byte, this requires double buffering and will have a significant impact on performance (e.g. in IoT use cases).

The problem can be fixed by performing type-conditional work with the provided io.Reader io.ByteScanner; in the case that the io.Reader is only an io.Reader document that the follow-up packet cannot be confirmed and just sync to the sync byte. If it is a Peeker, use that in the approach currently used. In the case that it is an io.ByteScanner iterate the bytes, reading until PacketLength and unreading when the sync byte is found.

[edit: cannot be a pure io.Reader under any circumstance]

kortschak commented 6 years ago

Looking further into it, I'm wondering why the function is not just this

func Sync(r io.ByteScanner) (off int64, err error) {
    for {
        b, err := r.ReadByte()
        if err == io.EOF {
            return off, gots.ErrSyncByteNotFound
        }
        if err != nil {
            return off, err
        }
        if b != SyncByte {
            off++
            continue
        }
        r.UnreadByte()
        return off, nil
    }
}

The current code does a lot of work and will incorrectly return a not found when an initial sync has been found but no following sync exists. This would be expected in the case of the last packet. The code here is slow, but I would expect much less slow than what currently exists.

Am I missing something fundamental here?

tmm1 commented 5 years ago

Can be closed?

kortschak commented 5 years ago

It can. Thanks.