Streaming sample getters make one file read per sample

Open
  • Is something wrong?
#709 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
30/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Active
Tech stack
go
Domain
performance

Research direction

Start with the per-sample ReadData calls in mp4/stream.go near lines 383 and 448, then the lazy read path in mp4/mdat.go around lines 244-252, and the existing grouped route in appendSamples. Run the reproduction script in the issue to confirm the baseline of 240 reads for GetSamples and GetSampleRange against 4 for the Append variants. Done means the getters make one read per contiguous run with the same SHA-256 and sample count, while getter errors and independent sample buffers still behave as before.

Written by the indexing model from the issue text.

Description

Streaming callers using InitDecodeStream with an unbuffered file and SampleAccessor.GetSamples or GetSampleRange read each selected sample separately, even when the bytes are contiguous. This affects callers that repackage or refragment through these getters.

On the committed mp4/testdata/v300_multiple_segments.mp4 at d98f0d6, a local comparison of existing APIs, not a patch, gave:

API Sample-access os.File.Read calls Local complete-operation median
GetSamples 240 826.828 µs
AppendSamples(nil, nil, trackID) 4 211.483 µs

The timing covers file open, stream decoding, extraction, building new fragments with AddFullSamples, encoding to io.Discard, and close: 15 interleaved pairs × 500 operations per arm. This includes allocation/layout differences, without append-buffer reuse. File handles were fresh, not the OS cache. Production impact is unmeasured.

GetSampleRange also made 240 reads; AppendSampleRange made four. All four returned 240 samples and 134,386 payload bytes with the same SHA-256 (below). Counts exclude parsing.

Likely cause: the getters call ReadData per sample (mp4/stream.go:383,448). Lazy ReadData seeks and creates a buffer (mp4/mdat.go:244–252); BoxSeekReader does not read ahead. The grouped route already exists in appendSamples. #594 explicitly left getter reimplementation for later.

Reproduce the read counts

From the repository root, save the following as /tmp/stream-samples.go and run go run /tmp/stream-samples.go. This opens the real fixture afresh for each API and counts reads during sample access.

package main

import (
	"crypto/sha256"
	"fmt"
	"os"

	"github.com/Eyevinn/mp4ff/mp4"
)

type countedFile struct {
	*os.File
	reads int
}

func (f *countedFile) Read(p []byte) (int, error) {
	f.reads++
	return f.File.Read(p)
}

func main() {
	for _, mode := range []string{"GetSamples", "GetSampleRange", "AppendSamples", "AppendSampleRange"} {
		f, err := os.Open("mp4/testdata/v300_multiple_segments.mp4")
		if err != nil {
			panic(err)
		}
		r := &countedFile{File: f}
		h := sha256.New()
		reads, count, size := 0, 0, 0
		sf, err := mp4.InitDecodeStream(r, mp4.WithFragmentCallback(func(frag *mp4.Fragment, sa mp4.SampleAccessor) error {
			for _, traf := range frag.Moof.Trafs {
				id := traf.Tfhd.TrackID
				var samples []mp4.FullSample
				var err error
				before := r.reads
				switch mode {
				case "GetSamples":
					samples, err = sa.GetSamples(id)
				case "GetSampleRange":
					samples, err = sa.GetSampleRange(id, 1, ^uint32(0))
				case "AppendSamples":
					samples, _, err = sa.AppendSamples(nil, nil, id)
				case "AppendSampleRange":
					samples, _, err = sa.AppendSampleRange(nil, nil, id, 1, ^uint32(0))
				}
				reads += r.reads - before
				if err != nil {
					return err
				}
				for _, s := range samples {
					count++
					size += len(s.Data)
					h.Write(s.Data)
				}
			}
			return nil
		}))
		if err == nil {
			err = sf.ProcessFragments()
		}
		closeErr := f.Close()
		if err != nil {
			panic(err)
		}
		if closeErr != nil {
			panic(closeErr)
		}
		fmt.Printf("%s reads=%d samples=%d bytes=%d sha256=%x\n", mode, reads, count, size, h.Sum(nil))
	}
}

Observed output:

GetSamples reads=240 samples=240 bytes=134386 sha256=3449633b656952612c827c0232a94ee21713a14c1460ca982bda9ef147c42a57
GetSampleRange reads=240 samples=240 bytes=134386 sha256=3449633b656952612c827c0232a94ee21713a14c1460ca982bda9ef147c42a57
AppendSamples reads=4 samples=240 bytes=134386 sha256=3449633b656952612c827c0232a94ee21713a14c1460ca982bda9ef147c42a57
AppendSampleRange reads=4 samples=240 bytes=134386 sha256=3449633b656952612c827c0232a94ee21713a14c1460ca982bda9ef147c42a57

The fixture has four fragments and four track runs. The desired improvement is four run reads instead of 240 sample reads on this input, while preserving getter errors and independent sample-buffer use. These counts do not establish behavior for cached rereads or failing inputs.

Would you like a pull request to coalesce contiguous getter reads while preserving those semantics?

Case and evidence

Made by Perfloop. Human sponsor: Tomás Senart.

Dominant language
Go
Stars
667
Forks
132
Avg merge
20h 4m
Merged PRs (30d)
54

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from Eyevinn/mp4ff

All issues in Eyevinn/mp4ff

Similar issues

More Go issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.