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

h264/bits: new BitReader to eventually replace BitReader in read.go #36

Merged
merged 28 commits into from
Jul 13, 2019

Conversation

saxon-milton
Copy link
Member

This is working towards resolving issue #30.

…t fields and improved formatting of NewBitreader
This change included modification to cache, specifically a rename of cache.next to cache.get.
This is not needed, BitReader.ReadBits(8) can be used
Reverted changes that I made to files cabac.go, nalUnit.go, rbsp.go, read.go and slice.go, as the changes would make this PR too large.
@shawnps shawnps mentioned this pull request Jul 10, 2019
Copy link
Collaborator

@kortschak kortschak left a comment

Choose a reason for hiding this comment

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

Looking at the complexity of this, I think you'd be better taking the code in bzip2 and adding a PeekBits. That is a trivial addition, using essentially the same code that is in ReadBits, but first Peeking the bytes to need to read the given number of bits. That would be (untested):

func (br *bitReader) PeekBits64(bits uint) (n uint64) {
	byt, err := br.r.Peek(int(n+7) / 8)
	if err != nil {
		if err == io.EOF {
			err = io.ErrUnexpectedEOF
		}
		br.err = err
		return 0
	}
	for i := 0; bits > br.bits; i++ {
		b := byt[i]
		if err != nil {
			br.err = err
			return 0
		}
		br.n <<= 8
		br.n |= uint64(b)
		br.bits += 8
	}

	n = (br.n >> (br.bits - bits)) & ((1 << bits) - 1)
	br.bits -= bits
	return
}

This requires that the copied version of bit_reader.go has the io.ByteReader field changed to either a *bufio.Reader or a local interface type interface { io.Reader; Peek(n int) ([]byte, error) } and do the appropriate check during construction of the bit reader.

h264/bits/bitreader.go Outdated Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
Copy link
Member Author

@saxon-milton saxon-milton left a comment

Choose a reason for hiding this comment

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

Looking at the complexity of this, I think you'd be better taking the code in bzip2 and adding a PeekBits. That is a trivial addition, using essentially the same code that is in ReadBits, but first Peeking the bytes to need to read the given number of bits. That would be (untested):

func (br *bitReader) PeekBits64(bits uint) (n uint64) {
	byt, err := br.r.Peek(int(n+7) / 8)
	if err != nil {
		if err == io.EOF {
			err = io.ErrUnexpectedEOF
		}
		br.err = err
		return 0
	}
	for i := 0; bits > br.bits; i++ {
		b := byt[i]
		if err != nil {
			br.err = err
			return 0
		}
		br.n <<= 8
		br.n |= uint64(b)
		br.bits += 8
	}

	n = (br.n >> (br.bits - bits)) & ((1 << bits) - 1)
	br.bits -= bits
	return
}

This requires that the copied version of bit_reader.go has the io.ByteReader field changed to either a *bufio.Reader or a local interface type interface { io.Reader; Peek(n int) ([]byte, error) } and do the appropriate check during construction of the bit reader.

Do I need to do any acknowledgement of the original code if I use this ?

h264/bits/bitreader.go Outdated Show resolved Hide resolved
@kortschak
Copy link
Collaborator

The bit_reader.go code is in the standard library. The standard approaches to using third party code apply.

@saxon-milton
Copy link
Member Author

The bit_reader.go code is in the standard library. The standard approaches to using third party code apply.

so I need to put this in the file header:
Copyright (c) 2009 The Go Authors. All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are
met:

  • Redistributions of source code must retain the above copyright
    notice, this list of conditions and the following disclaimer.
  • Redistributions in binary form must reproduce the above
    copyright notice, this list of conditions and the following disclaimer
    in the documentation and/or other materials provided with the
    distribution.
  • Neither the name of Google Inc. nor the names of its
    contributors may be used to endorse or promote products derived from
    this software without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

??

…pkg, also wrote tests for a series of read and peek operations.
h264/bits/bitreader.go Show resolved Hide resolved
Saxon Nelson-Milton <saxon@ausocean.org>, The Australian Ocean Laboratory (AusOcean)

LICENSE

Copy link
Collaborator

Choose a reason for hiding this comment

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

This license only applies to portions (admittedly major portions). You should also include whatever license we are applying to this.

Say which file it was derived from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Say which file the code was derived from ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you really use two licenses on the same file ? What if conditions in the licenses clash ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the licenses clash, you can't use them together. That's not limited to file scope. Everything that uses the standard library (i.e. all Go code since runtime is in the standard library) uses the Go license. This just makes it explicit that this code is from there.

Copy link
Member Author

Choose a reason for hiding this comment

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

How does the copyright work ? Do I add the ausocean copyright too ?

h264/bits/bitreader.go Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
h264/bits/bitreader_test.go Outdated Show resolved Hide resolved
h264/bits/bitreader_test.go Outdated Show resolved Hide resolved
h264/bits/bitreader_test.go Outdated Show resolved Hide resolved
h264/bits/bitreader_test.go Outdated Show resolved Hide resolved
h264/bits/bitreader_test.go Outdated Show resolved Hide resolved
h264/bits/bitreader.go Outdated Show resolved Hide resolved
@kortschak
Copy link
Collaborator

kortschak commented Jul 12, 2019

I would have abandoned this and started a new PR, but it is what it is. Please squash when you merge.

Copy link
Collaborator

@kortschak kortschak left a comment

Choose a reason for hiding this comment

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

Please squash when you merge.

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

Successfully merging this pull request may close these issues.

None yet

2 participants