Seek #43

cswank · 2021-01-17T16:30:58Z

WIP!!!

I got something working with seek table and fixed block sized flac. I'm making a PR now just to see if you think I'm on the right track before going too far.

If the Flac file doesn't have a seek table in its metadata then this builds one. I still need to make it work for FixedBlockSize=false Flac files (gotta find a file like this first).

got something working with seek table and fixed block sized flac

karlek · 2021-01-17T22:03:24Z

Hi cswank!

Glad to see someone giving seeking some well-earned love. I'll check the PR tomorrow 👍

TODO: handle cases where sample is invalid: whence == io.SeekEnd && sample is not negative whence == io.SeekStart && sample is negative whence == io.SeekCurrent and sample + current sample < 0 or > stream.Info.NSamples

cswank · 2021-01-18T14:49:30Z

The last commit supports io.SeekCurrent and io.SeekEnd

cswank · 2021-01-18T14:49:51Z

flac.go

-	return 0, nil
+// Seek to offset where offset represents the sample number in the flac file
+//
+// TODO: return an error if sample is invalid?  Do nothing?


How do you think invalid input to Seek should be handled?

Your comment about when the sample is invalid is perfect. I think the best would be to create a new error type for those cases. For example:

ErrInvalidSeek = errors.New("stream.Seek: out of stream seek")

We have custom error types in meta.go which are public so that library end-users may check against them.

cswank · 2021-01-18T14:52:05Z

flac.go

+			return err
+		}
+
+		if i%10 == 0 {


I made the assumption that the number of seek points can be reduced in order to save memory and increase seek performance (don't have to search through so many points in the seek table) at the cost of the seeks having lower resolution. Does this seem appropriate?

Yes, that's good!

I don't have a good gut feeling what the perfect resolution is, since this library could possibly handle short sound segments (<2s) to long sound segments (>10h+). This could possibly be a configure option, but let's just go with %10 for now.

Or do you have any opinions @mewmew ?

We could use a sane default (e.g. 10% as you suggested), and then at a later point introduce a public API for initializing a custom seek table at arbitrary resolution (that API point would essentially set the unexported seekTable field).

flac.go

cswank · 2021-01-18T15:01:49Z

flac.go

+	}
+
+	if seeker && stream.seekTable == nil {
+		return stream, stream.makeSeekTable()


Perhaps makeSeekTable should be called when Seek is called for the first time?

For the flac file I've been playing makeSeekTable takes 1.1 seconds, which is kind of a lot.

That's a good suggestion. Seek should probably be an opt-in feature, and we want to keep simple parsing as streamlined as possible.

It's easy to check if makeSeekTable has been called by checking if stream.seekTable == nil or not.

Perhaps makeSeekTable should be called when Seek is called for the first time?

This sounds like a good approach. And then if we introduce an API such as InitSeekTable, then the user could ensure that the seek table is already initialized should they wish before the first call to Seek.

karlek

Really like your implementation, just a few kinks we need to straighten out before we merge this. Thank you so much for contributing to our repo 🌷

Also, would it be possible to write a test for Seek testing SeekStart, SeekCurrent and SeekEnd with any of the flac files in this repo?

Note: I use the conventional comments format when I review, if you're confused just ask a question and I'll respond as soon as I can or read more about it here: https://conventionalcomments.org/

karlek · 2021-01-18T23:46:06Z

flac.go

@@ -48,6 +48,11 @@ type Stream struct {
 	// Zero or more metadata blocks.
 	Blocks []*meta.Block
 	// Underlying io.Reader.
+
+	seekTable *meta.SeekTable
+	// offset where the music data begins since SeekTable.Offset seems to be relative to this position


nitpick: Capital letter and punctuation for comments.

suggestion: rename start to dataStart or something more descriptive.

karlek · 2021-01-18T23:48:53Z

flac.go

-	br := bufio.NewReader(r)
-	stream = &Stream{r: br}
-	isLast, err := stream.parseStreamInfo()
+	//br := bufio.NewReader(r)


issue(blocking): This is a breaking change, since parsing would no longer be buffered. This can cause performance and stability issues for users of this library.

Reverting stream.r back to bufio.Reader means losing access to the Seeker (stream.r.(io.ReadSeeker) will fail). I'm trying to think of a way around this, so stay tuned.

karlek · 2021-01-18T23:55:03Z

flac.go

@@ -94,41 +121,41 @@ var id3Signature = []byte("ID3")
 // stream, and parses the StreamInfo metadata block. It returns a boolean value
 // which specifies if the StreamInfo block was the last metadata block of the
 // FLAC stream.
-func (stream *Stream) parseStreamInfo() (isLast bool, err error) {
+func (stream *Stream) parseStreamInfo() (block *meta.Block, err error) {


praise: Nice refactor, block.IsLast reads even better in New and exposes SeekTable 👍

thought: When I think about it, stream's parseStreamInfo has a name conflict with block's parseStreamInfo, maybe we should rename one of them. What do you think @mewmew?

thought: When I think about it, stream's parseStreamInfo has a name conflict with block's parseStreamInfo, maybe we should rename one of them. What do you think @mewmew?

Both are unexported so we can rename without changing exported API. Personally, I think it is fine as is, but would also be fine with changing the name of one to avoid confusion. The are distinct by one being a function, the other a method, so the call sites will be different. If we do change the name, e.g. of the method parseStreamInfo the names of other methods parsing blocks should be updated for consistency.

karlek · 2021-01-18T23:56:55Z

flac.go

-	br := bufio.NewReader(r)
-	stream = &Stream{r: br}
-	isLast, err := stream.parseStreamInfo()
+	stream = &Stream{r: r}


issue(blocking): This is a breaking change, since parsing would no longer be buffered. This can cause performance and stability issues for users of this library.

karlek · 2021-01-19T00:05:17Z

flac.go

@@ -157,18 +181,17 @@ func (stream *Stream) skipID3v2() error {
 //
 // Call Stream.Next to parse the frame header of the next audio frame, and call
 // Stream.ParseNext to parse the entire next frame including audio samples.
-func Parse(r io.Reader) (stream *Stream, err error) {
+func Parse(r io.ReadSeeker) (stream *Stream, err error) {


issue(blocking): Calls to flac.Parse won't implement seeking. Since the metadata block will be parsed, but the SeekTable won't be created. The logic from New is missing here to correctly implement seeking in both places.

End-users of this library, should be able to seek regardless if they initialized with New or Parse

thought: The names New and Parse don't accurately reflect what they do. At least the functionality is well explained in the documentation. What did we think here @mewmew haha?

karlek · 2021-01-19T01:02:33Z

flac.go

+		return 0, nil
+	}
+
+	r := stream.r.(io.ReadSeeker)


nitpick: This shadows r, use rs as you did in New.

karlek · 2021-01-19T01:04:19Z

flac.go

+}
+
+func (stream *Stream) makeSeekTable() (err error) {
+	r := stream.r.(io.ReadSeeker)


nitpick: This shadows r, use rs as you did in New.

karlek · 2021-01-19T01:08:39Z

flac.go

+			return err
+		}
+
+		if i%10 == 0 {


Yes, that's good!

I don't have a good gut feeling what the perfect resolution is, since this library could possibly handle short sound segments (<2s) to long sound segments (>10h+). This could possibly be a configure option, but let's just go with %10 for now.

Or do you have any opinions @mewmew ?

karlek · 2021-01-19T01:09:52Z

flac.go

+		}
+
+		if i%10 == 0 {
+			o, err := r.Seek(0, io.SeekCurrent)


question: Second time I see this pattern, wondering if it would be better to create a function called something like currentOffset?

Sure, no problem if you really want this. However wouldn't that func have to do another stream.r.(io.ReadSeeker) assertion? So it'd either look like:

func (stream *Stream) currentOffset() (int64, error) { rs, ok := stream.r.(io.ReadSeeker) if !ok { return 0, ErrNoSeeker } return rs.Seek(0, io.SeekCurrent) }

Or

func (stream *Stream) currentOffset(rs io.ReadSeeker) (int64, error) { return rs.Seek(0, io.SeekCurrent) }

I kind of like the way it is compared to either of those funcs, but am happy to make the change if you want.

Yeah, it doesn't look that much better. Keep it as it is! Thanks for writing it out :)

flac.go

cswank · 2021-01-19T13:49:29Z

flac.go

+	if seeker {
+		stream.r = r
+	} else {
+		stream.r = bufio.NewReader(r)


What do you think about this change?

Here is the reasoning:

if stream.r is *bufio.Reader then it is no longer possible to assert for io.ReadSeeker and so Seek is no longer possible.

if r is io.ReadSeeker then r must be either *os.File or an in-memory buffer of some kind.

if r is not io.ReadSeeker then it is probably a network stream, like from http.Request.Body???

My guess is that bufio.Reader is really only needed when the reader is streaming over a network. If reader is *os.File or it's an in memory buffer then bufio.Reader really isn't necessary (but you'll have a way better feel for this than I)

What do you think about this change?

The change looks good to me, and the reasoning valid.

My guess is that bufio.Reader is really only needed when the reader is streaming over a network. If reader is *os.File or it's an in memory buffer then bufio.Reader really isn't necessary.

I think we did some preliminary tests using *os.File without buffered reading and it did (at least for an older version of mewkiz/flac) impact the performance. This was the primary reason we used buffered reading as default.

If *os.File did cause problems then it seems like explicitly opting in for the ability to Seek is the thing to do.

something like?

`type Option func(stream *Stream) (err error)

func New(r io.Reader, opts ...Option) (stream *Stream, err error) {
...
}

func EnableSeek(stream *Stream) (err error) {
// don't make stream.r a *bufio.Reader and look for seekTable in metadata
...
}
`

mewmew · 2021-01-19T14:23:58Z

flac.go

@@ -48,6 +49,11 @@ type Stream struct {
 	// Zero or more metadata blocks.
 	Blocks []*meta.Block
 	// Underlying io.Reader.
+
+	seekTable *meta.SeekTable


Suggested change

seekTable *meta.SeekTable

// seekTable contains one or more pre-calculated audio frame seek points of the stream; nil if uninitialized.

seekTable *meta.SeekTable

mewmew · 2021-01-19T14:26:26Z

flac.go

@@ -48,6 +49,11 @@ type Stream struct {
 	// Zero or more metadata blocks.
 	Blocks []*meta.Block
 	// Underlying io.Reader.
+
+	seekTable *meta.SeekTable
+	// Offset where the music data begins since SeekTable.Offset seems to be relative to this position.


since SeekTable.Offset seems to be relative to this position.

We should try to verify this against the official spec and then update the comment accordingly (e.g. remove "seems").

I have been testing with a single Flac file that has a seekTable and its first data point is meta.SeekPoint{SampleNum:0, Offset:0}.

"Offset (in bytes) from the first byte of the first frame header to the first byte of the target frame's header."

from https://xiph.org/flac/format.html#metadata_block_seektable, so I removed 'seems'

mewmew · 2021-01-19T14:30:59Z

flac.go

 	}

 	return stream, nil
 }

-// flacSignature marks the beginning of a FLAC stream.
-var flacSignature = []byte("fLaC")
+// func EnsureSeekTable(stream *Stream) error {


The final PR should probably remove this commented-out lines?

mewmew · 2021-01-19T14:38:02Z

flac.go

+			return p
+		}
+	}
+	return p


It wasn't immediately clear to me that this line returned the zero value. For clarity, consider returning meta.SeekPoint{} which stands out as a zero value, and thus not the "intended" code path.

Then, the function would also be updated to not used named returned values. While this function is not exported it doesn't matter too much. For exported API however, one suggestion is to only used named returns when they help support documentation, e.g. when a function returns two integers (x, y int).

This would return the last SeekPoint. I've been playing with ReadSeeker a bit and returning the last point actually seems to mimic the behavior seen here:

https://play.golang.org/p/WStOyCU2wU9

In other words, seeking before the start isn't an error seeking past the end isn't either.

cswank · 2021-01-19T20:48:16Z

@mewmew and @karlek What do you think about?0457be3

As I said in the commit message, I won't be offended if you don't want this.

This commit changes the way Stream is initialized quite a bit and maybe it is less readable? I personally think it's more readable because I really like the variadic argument pattern so I've used it quite a bit. I think it addresses some of the concerns: 1: Use *bufio.Reader as the default in order to match the current behaivor by calling stream, err = flac.New(r) 2: Make Seek explicitly optional by calling: stream, err = flac.New(r, flac.EnableSeek)

mewmew · 2021-01-21T09:07:52Z

Hi @cswank,

Just a quick heads up. My brother @karlek and I will look at this PR when we find the time to see each other (probably this weekend or next week). Now, we're both busy with work + Uni :)

Thanks a lot for working on the seek support for FLAC, it's really been the last missing feature of the low level API for quite some time. This would mark the perfect transition for discussing the v2.0 API (#33), to split the API into two parts; one high-level user facing, and one low-level API of FLAC.

After a brief discussion with Henry, I think we will try to limit the scope of this PR to functionality changes - in other words the core seek functionality - and then move extended API discussions (e.g. adding Option func(s *Stream, r io.Reader) error to an issue, where we can reach a consensus on a good and consistent approach to use for the API also going forward. This would tie well into the v2.0 API discussion linked above, and we would warmly welcome you to join the discussions Craig. It's always good to have multiple views and different background when designing an API.

Cheers,
Robin

cswank · 2021-01-21T12:32:41Z

Hi Robin,
That sounds like a really good idea. The PR was starting to spread out into multiple areas. I almost feel like I should close this PR and start over with seek only and no api changes as you suggest. What do you think? The new PR could just add NewSeek (instead of modifying New) and leave the current functionality alone, which would reduce the chances of introducing a breaking change.

EDIT: I have a new branch that I haven't pushed up yet for a new PR.

Craig

mewmew · 2021-01-21T16:59:23Z

That sounds like a really good idea. The PR was starting to spread out into multiple areas. I almost feel like I should close this PR and start over with seek only and no api changes as you suggest. What do you think? The new PR could just add NewSeek (instead of modifying New) and leave the current functionality alone, which would reduce the chances of introducing a breaking change.

Sounds great!

cswank added 5 commits January 17, 2021 09:21

wip

234e5d1

got something working with seek table and fixed block sized flac

Don't need readseeker receiver

81242fa

consider the number of subframes

e16fd49

return SampleNum instead of file offset

b4c1a69

sane-ish number of seek points

de12b38

support io.SeekEnd and io.SeekCurrent

f6950a1

TODO: handle cases where sample is invalid: whence == io.SeekEnd && sample is not negative whence == io.SeekStart && sample is negative whence == io.SeekCurrent and sample + current sample < 0 or > stream.Info.NSamples

cswank commented Jan 18, 2021

View reviewed changes

flac.go Show resolved Hide resolved

cswank commented Jan 18, 2021

View reviewed changes

cswank added 2 commits January 18, 2021 09:06

hande seek error

230217d

whence error

c28c6a4

lxea mentioned this pull request Jan 18, 2021

Jukebox Improvments: Support seeking, fix the default returned return api type sentriz/gonic#109

Merged

karlek requested changes Jan 19, 2021

View reviewed changes

CR changes

38f6396

cswank commented Jan 19, 2021

View reviewed changes

cswank added 4 commits January 19, 2021 06:57

parse seekTable in flac.Parse

e05b8a8

use bufio.Reader in flac.Parse

ef305be

more CR changes

df42987

fix searchFromEnd

9c8b366

mewmew reviewed Jan 19, 2021

View reviewed changes

cswank added 6 commits January 19, 2021 07:40

fix comments

9fe78ac

remote commented out code

1fb180a

remove 'seems'

dcd1fe4

fix comment

ad8e6e7

comment

c28ed01

reword comment

190d6e3

cswank force-pushed the seek branch from 768ed1b to 050bf4d Compare January 20, 2021 12:36

cswank force-pushed the seek branch from 050bf4d to 0457be3 Compare January 20, 2021 12:37

cswank added 6 commits January 20, 2021 06:03

rename Option func

17be08e

wip

3a3d69e

add skip test

58790bb

more seeks in seek test

9433f10

test decode all the test files

61be5f2

close test stream

266a720

cswank closed this Jan 21, 2021

mewmew mentioned this pull request Jan 23, 2021

Update seek API to use absolute rather than relative sample number #45

Closed

	seekTable *meta.SeekTable
	// seekTable contains one or more pre-calculated audio frame seek points of the stream; nil if uninitialized.
	seekTable *meta.SeekTable

Seek #43

Seek #43

Conversation

cswank commented Jan 17, 2021 • edited Loading

karlek commented Jan 17, 2021

cswank commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cswank Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cswank Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlek Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

cswank Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cswank commented Jan 19, 2021 • edited Loading

mewmew commented Jan 21, 2021

cswank commented Jan 21, 2021 • edited Loading

mewmew commented Jan 21, 2021

cswank commented Jan 17, 2021 •

edited

Loading

cswank Jan 18, 2021 •

edited

Loading

cswank Jan 18, 2021 •

edited

Loading

karlek left a comment •

edited

Loading

karlek Jan 19, 2021 •

edited

Loading

cswank Jan 19, 2021 •

edited

Loading

cswank commented Jan 19, 2021 •

edited

Loading

cswank commented Jan 21, 2021 •

edited

Loading