[bugfix] Parse video metadata more accurately; allow Range in fileserver (#1342)

* don't serve unused fields for video attachments

* parse video bitrate + duration more accurately

* use ServeContent where appropriate to respect Range

* abstract temp file seeker into its own function

internal/api/client/media/mediacreate_test.go

@@ -201,7 +201,7 @@ func (suite *MediaCreateTestSuite) TestMediaCreateSuccessful() {
 			Size:   "512x288",
 			Aspect: 1.7777778,
 		},
-		Focus: apimodel.MediaFocus{
+		Focus: &apimodel.MediaFocus{
 			X: -0.5,
 			Y: 0.5,
 		},
@@ -290,7 +290,7 @@ func (suite *MediaCreateTestSuite) TestMediaCreateSuccessfulV2() {
 			Size:   "512x288",
 			Aspect: 1.7777778,
 		},
-		Focus: apimodel.MediaFocus{
+		Focus: &apimodel.MediaFocus{
 			X: -0.5,
 			Y: 0.5,
 		},

internal/api/client/media/mediaupdate_test.go

@@ -172,7 +172,7 @@ func (suite *MediaUpdateTestSuite) TestUpdateImage() {
 	suite.EqualValues(apimodel.MediaMeta{
 		Original: apimodel.MediaDimensions{Width: 800, Height: 450, FrameRate: "", Duration: 0, Bitrate: 0, Size: "800x450", Aspect: 1.7777778},
 		Small:    apimodel.MediaDimensions{Width: 256, Height: 144, FrameRate: "", Duration: 0, Bitrate: 0, Size: "256x144", Aspect: 1.7777778},
-		Focus:    apimodel.MediaFocus{X: -0.1, Y: 0.3},
+		Focus:    &apimodel.MediaFocus{X: -0.1, Y: 0.3},
 	}, attachmentReply.Meta)
 	suite.Equal(toUpdate.Blurhash, attachmentReply.Blurhash)
 	suite.Equal(toUpdate.ID, attachmentReply.ID)

internal/api/fileserver/servefile.go

@@ -29,6 +29,7 @@
 	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
 	apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+	"github.com/superseriousbusiness/gotosocial/internal/iotools"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/oauth"
 )
@@ -128,8 +129,34 @@ func (m *Module) ServeFile(c *gin.Context) {
 		return
 	}
 
-	// we're good, return the slurped bytes + the rest of the content
-	c.DataFromReader(http.StatusOK, content.ContentLength, format, io.MultiReader(
-		bytes.NewReader(b), content.Content,
-	), nil)
+	// reconstruct the original content reader
+	r := io.MultiReader(bytes.NewReader(b), content.Content)
+
+	// Check the Range header: if this is a simple query for the whole file, we can return it now.
+	if c.GetHeader("Range") == "" && c.GetHeader("If-Range") == "" {
+		c.DataFromReader(http.StatusOK, content.ContentLength, format, r, nil)
+		return
+	}
+
+	// Range is set, so we need a ReadSeeker to pass to the ServeContent function.
+	tfs, err := iotools.TempFileSeeker(r)
+	if err != nil {
+		err = fmt.Errorf("ServeFile: error creating temp file seeker: %w", err)
+		apiutil.ErrorHandler(c, gtserror.NewErrorInternalError(err), m.processor.InstanceGet)
+		return
+	}
+	defer func() {
+		if err := tfs.Close(); err != nil {
+			log.Errorf("ServeFile: error closing temp file seeker: %s", err)
+		}
+	}()
+
+	// to avoid ServeContent wasting time seeking for the
+	// mime type, set this header already since we know it
+	c.Header("Content-Type", format)
+
+	// allow ServeContent to handle the rest of the request;
+	// it will handle Range as appropriate, and write correct
+	// response headers, http code, etc
+	http.ServeContent(c.Writer, c.Request, fileName, content.ContentUpdated, tfs)
 }

internal/api/model/attachment.go

@@ -98,40 +98,12 @@ type Attachment struct {
 //
 // swagger:model mediaMeta
 type MediaMeta struct {
-	Length string `json:"length,omitempty"`
-	// Duration of the media in seconds.
-	// Only set for video and audio.
-	// example: 5.43
-	Duration float32 `json:"duration,omitempty"`
-	// Framerate of the media.
-	// Only set for video and gifs.
-	// example: 30
-	FPS uint16 `json:"fps,omitempty"`
-	// Size of the media, in the format `[width]x[height]`.
-	// Not set for audio.
-	// example: 1920x1080
-	Size string `json:"size,omitempty"`
-	// Width of the media in pixels.
-	// Not set for audio.
-	// example: 1920
-	Width int `json:"width,omitempty"`
-	// Height of the media in pixels.
-	// Not set for audio.
-	// example: 1080
-	Height int `json:"height,omitempty"`
-	// Aspect ratio of the media.
-	// Equal to width / height.
-	// example: 1.777777778
-	Aspect        float32 `json:"aspect,omitempty"`
-	AudioEncode   string  `json:"audio_encode,omitempty"`
-	AudioBitrate  string  `json:"audio_bitrate,omitempty"`
-	AudioChannels string  `json:"audio_channels,omitempty"`
 	// Dimensions of the original media.
 	Original MediaDimensions `json:"original"`
 	// Dimensions of the thumbnail/small version of the media.
 	Small MediaDimensions `json:"small,omitempty"`
 	// Focus data for the media.
-	Focus MediaFocus `json:"focus,omitempty"`
+	Focus *MediaFocus `json:"focus,omitempty"`
 }
 
 // MediaFocus models the focal point of a piece of media.

internal/api/model/content.go

@@ -21,6 +21,7 @@
 import (
 	"io"
 	"net/url"
+	"time"
 )
 
 // Content wraps everything needed to serve a blob of content (some kind of media) through the API.
@@ -29,6 +30,8 @@ type Content struct {
 	ContentType string
 	// ContentLength in bytes
 	ContentLength int64
+	// Time when the content was last updated.
+	ContentUpdated time.Time
 	// Actual content
 	Content io.ReadCloser
 	// Resource URL to forward to if the file can be fetched from the storage directly (e.g signed S3 URL)

internal/iotools/io.go

@@ -20,6 +20,7 @@
 
 import (
 	"io"
+	"os"
 )
 
 // ReadFnCloser takes an io.Reader and wraps it to use the provided function to implement io.Closer.
@@ -157,3 +158,35 @@ func StreamWriteFunc(write func(io.Writer) error) io.Reader {
 
 	return pr
 }
+
+type tempFileSeeker struct {
+	io.Reader
+	io.Seeker
+	tmp *os.File
+}
+
+func (tfs *tempFileSeeker) Close() error {
+	tfs.tmp.Close()
+	return os.Remove(tfs.tmp.Name())
+}
+
+// TempFileSeeker converts the provided Reader into a ReadSeekCloser
+// by using an underlying temporary file. Callers should call the Close
+// function when they're done with the TempFileSeeker, to release +
+// clean up the temporary file.
+func TempFileSeeker(r io.Reader) (io.ReadSeekCloser, error) {
+	tmp, err := os.CreateTemp(os.TempDir(), "gotosocial-")
+	if err != nil {
+		return nil, err
+	}
+
+	if _, err := io.Copy(tmp, r); err != nil {
+		return nil, err
+	}
+
+	return &tempFileSeeker{
+		Reader: tmp,
+		Seeker: tmp,
+		tmp:    tmp,
+	}, nil
+}

internal/media/manager_test.go

@@ -414,9 +414,9 @@ func (suite *ManagerTestSuite) TestSlothVineProcessBlocking() {
 	suite.Equal(240, attachment.FileMeta.Original.Height)
 	suite.Equal(81120, attachment.FileMeta.Original.Size)
 	suite.EqualValues(1.4083333, attachment.FileMeta.Original.Aspect)
-	suite.EqualValues(6.5862, *attachment.FileMeta.Original.Duration)
+	suite.EqualValues(6.640907, *attachment.FileMeta.Original.Duration)
 	suite.EqualValues(29.000029, *attachment.FileMeta.Original.Framerate)
-	suite.EqualValues(0x3b3e1, *attachment.FileMeta.Original.Bitrate)
+	suite.EqualValues(0x59e74, *attachment.FileMeta.Original.Bitrate)
 	suite.EqualValues(gtsmodel.Small{
 		Width: 338, Height: 240, Size: 81120, Aspect: 1.4083333333333334,
 	}, attachment.FileMeta.Small)
@@ -531,6 +531,82 @@ func (suite *ManagerTestSuite) TestLongerMp4ProcessBlocking() {
 	suite.Equal(processedThumbnailBytesExpected, processedThumbnailBytes)
 }
 
+func (suite *ManagerTestSuite) TestBirdnestMp4ProcessBlocking() {
+	ctx := context.Background()
+
+	data := func(_ context.Context) (io.ReadCloser, int64, error) {
+		// load bytes from a test video
+		b, err := os.ReadFile("./test/birdnest-original.mp4")
+		if err != nil {
+			panic(err)
+		}
+		return io.NopCloser(bytes.NewBuffer(b)), int64(len(b)), nil
+	}
+
+	accountID := "01FS1X72SK9ZPW0J1QQ68BD264"
+
+	// process the media with no additional info provided
+	processingMedia, err := suite.manager.ProcessMedia(ctx, data, nil, accountID, nil)
+	suite.NoError(err)
+	// fetch the attachment id from the processing media
+	attachmentID := processingMedia.AttachmentID()
+
+	// do a blocking call to fetch the attachment
+	attachment, err := processingMedia.LoadAttachment(ctx)
+	suite.NoError(err)
+	suite.NotNil(attachment)
+
+	// make sure it's got the stuff set on it that we expect
+	// the attachment ID and accountID we expect
+	suite.Equal(attachmentID, attachment.ID)
+	suite.Equal(accountID, attachment.AccountID)
+
+	// file meta should be correctly derived from the video
+	suite.Equal(404, attachment.FileMeta.Original.Width)
+	suite.Equal(720, attachment.FileMeta.Original.Height)
+	suite.Equal(290880, attachment.FileMeta.Original.Size)
+	suite.EqualValues(0.5611111, attachment.FileMeta.Original.Aspect)
+	suite.EqualValues(9.822041, *attachment.FileMeta.Original.Duration)
+	suite.EqualValues(30, *attachment.FileMeta.Original.Framerate)
+	suite.EqualValues(0x117c79, *attachment.FileMeta.Original.Bitrate)
+	suite.EqualValues(gtsmodel.Small{
+		Width: 287, Height: 512, Size: 146944, Aspect: 0.5605469,
+	}, attachment.FileMeta.Small)
+	suite.Equal("video/mp4", attachment.File.ContentType)
+	suite.Equal("image/jpeg", attachment.Thumbnail.ContentType)
+	suite.Equal(1409577, attachment.File.FileSize)
+	suite.Equal("L00000fQfQfQfQfQfQfQfQfQfQfQ", attachment.Blurhash)
+
+	// now make sure the attachment is in the database
+	dbAttachment, err := suite.db.GetAttachmentByID(ctx, attachmentID)
+	suite.NoError(err)
+	suite.NotNil(dbAttachment)
+
+	// make sure the processed file is in storage
+	processedFullBytes, err := suite.storage.Get(ctx, attachment.File.Path)
+	suite.NoError(err)
+	suite.NotEmpty(processedFullBytes)
+
+	// load the processed bytes from our test folder, to compare
+	processedFullBytesExpected, err := os.ReadFile("./test/birdnest-processed.mp4")
+	suite.NoError(err)
+	suite.NotEmpty(processedFullBytesExpected)
+
+	// the bytes in storage should be what we expected
+	suite.Equal(processedFullBytesExpected, processedFullBytes)
+
+	// now do the same for the thumbnail and make sure it's what we expected
+	processedThumbnailBytes, err := suite.storage.Get(ctx, attachment.Thumbnail.Path)
+	suite.NoError(err)
+	suite.NotEmpty(processedThumbnailBytes)
+
+	processedThumbnailBytesExpected, err := os.ReadFile("./test/birdnest-thumbnail.jpg")
+	suite.NoError(err)
+	suite.NotEmpty(processedThumbnailBytesExpected)
+
+	suite.Equal(processedThumbnailBytesExpected, processedThumbnailBytes)
+}
+
 func (suite *ManagerTestSuite) TestNotAnMp4ProcessBlocking() {
 	// try to load an 'mp4' that's actually an mkv in disguise
 
@@ -553,7 +629,7 @@ func (suite *ManagerTestSuite) TestNotAnMp4ProcessBlocking() {
 
 	// we should get an error while loading
 	attachment, err := processingMedia.LoadAttachment(ctx)
-	suite.EqualError(err, "error decoding video: error determining video metadata: [width height duration framerate bitrate]")
+	suite.EqualError(err, "error decoding video: error determining video metadata: [width height framerate]")
 	suite.Nil(attachment)
 }
 

internal/media/video.go

@@ -21,9 +21,10 @@
 import (
 	"fmt"
 	"io"
-	"os"
 
 	"github.com/abema/go-mp4"
+	"github.com/superseriousbusiness/gotosocial/internal/iotools"
+	"github.com/superseriousbusiness/gotosocial/internal/log"
 )
 
 type gtsVideo struct {
@@ -36,43 +37,48 @@ type gtsVideo struct {
 // decodeVideoFrame decodes and returns an image from a single frame in the given video stream.
 // (note: currently this only returns a blank image resized to fit video dimensions).
 func decodeVideoFrame(r io.Reader) (*gtsVideo, error) {
-	// We'll need a readseeker to decode the video. We can get a readseeker
-	// without burning too much mem by first copying the reader into a temp file.
-	// First create the file in the temporary directory...
-	tmp, err := os.CreateTemp(os.TempDir(), "gotosocial-")
+	// we need a readseeker to decode the video...
+	tfs, err := iotools.TempFileSeeker(r)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("error creating temp file seeker: %w", err)
 	}
-
 	defer func() {
-		tmp.Close()
-		os.Remove(tmp.Name())
+		if err := tfs.Close(); err != nil {
+			log.Errorf("error closing temp file seeker: %s", err)
+		}
 	}()
 
-	// Now copy the entire reader we've been provided into the
-	// temporary file; we won't use the reader again after this.
-	if _, err := io.Copy(tmp, r); err != nil {
-		return nil, err
-	}
-
 	// probe the video file to extract useful metadata from it; for methodology, see:
 	// https://github.com/abema/go-mp4/blob/7d8e5a7c5e644e0394261b0cf72fef79ce246d31/mp4tool/probe/probe.go#L85-L154
-	info, err := mp4.Probe(tmp)
+	info, err := mp4.Probe(tfs)
 	if err != nil {
-		return nil, fmt.Errorf("error probing tmp file %s: %w", tmp.Name(), err)
+		return nil, fmt.Errorf("error during mp4 probe: %w", err)
 	}
 
 	var (
-		width  int
-		height int
-		video  gtsVideo
+		width        int
+		height       int
+		videoBitrate uint64
+		audioBitrate uint64
+		video        gtsVideo
 	)
 
 	for _, tr := range info.Tracks {
 		if tr.AVC == nil {
+			// audio track
+			if br := tr.Samples.GetBitrate(tr.Timescale); br > audioBitrate {
+				audioBitrate = br
+			} else if br := info.Segments.GetBitrate(tr.TrackID, tr.Timescale); br > audioBitrate {
+				audioBitrate = br
+			}
+
+			if d := float64(tr.Duration) / float64(tr.Timescale); d > float64(video.duration) {
+				video.duration = float32(d)
+			}
 			continue
 		}
 
+		// video track
 		if w := int(tr.AVC.Width); w > width {
 			width = w
 		}
@@ -81,10 +87,10 @@ func decodeVideoFrame(r io.Reader) (*gtsVideo, error) {
 			height = h
 		}
 
-		if br := tr.Samples.GetBitrate(tr.Timescale); br > video.bitrate {
-			video.bitrate = br
-		} else if br := info.Segments.GetBitrate(tr.TrackID, tr.Timescale); br > video.bitrate {
-			video.bitrate = br
+		if br := tr.Samples.GetBitrate(tr.Timescale); br > videoBitrate {
+			videoBitrate = br
+		} else if br := info.Segments.GetBitrate(tr.TrackID, tr.Timescale); br > videoBitrate {
+			videoBitrate = br
 		}
 
 		if d := float64(tr.Duration) / float64(tr.Timescale); d > float64(video.duration) {
@@ -93,6 +99,10 @@ func decodeVideoFrame(r io.Reader) (*gtsVideo, error) {
 		}
 	}
 
+	// overall bitrate should be audio + video combined
+	// (since they're both playing at the same time)
+	video.bitrate = audioBitrate + videoBitrate
+
 	// Check for empty video metadata.
 	var empty []string
 	if width == 0 {

internal/processing/media/getfile.go

@@ -85,9 +85,6 @@ func (p *processor) GetFile(ctx context.Context, requestingAccount *gtsmodel.Acc
 }
 
 func (p *processor) getAttachmentContent(ctx context.Context, requestingAccount *gtsmodel.Account, wantedMediaID string, owningAccountID string, mediaSize media.Size) (*apimodel.Content, gtserror.WithCode) {
-	attachmentContent := &apimodel.Content{}
-	var storagePath string
-
 	// retrieve attachment from the database and do basic checks on it
 	a, err := p.db.GetAttachmentByID(ctx, wantedMediaID)
 	if err != nil {
@@ -146,6 +143,13 @@ func (p *processor) getAttachmentContent(ctx context.Context, requestingAccount
 		}
 	}
 
+	var (
+		storagePath       string
+		attachmentContent = &apimodel.Content{
+			ContentUpdated: a.UpdatedAt,
+		}
+	)
+
 	// get file information from the attachment depending on the requested media size
 	switch mediaSize {
 	case media.SizeOriginal:

internal/typeutils/internaltofrontend.go

@@ -284,19 +284,13 @@ func (c *converter) AttachmentToAPIAttachment(ctx context.Context, a *gtsmodel.M
 			Original: apimodel.MediaDimensions{
 				Width:  a.FileMeta.Original.Width,
 				Height: a.FileMeta.Original.Height,
-				Size:   fmt.Sprintf("%dx%d", a.FileMeta.Original.Width, a.FileMeta.Original.Height),
-				Aspect: float32(a.FileMeta.Original.Aspect),
 			},
 			Small: apimodel.MediaDimensions{
 				Width:  a.FileMeta.Small.Width,
 				Height: a.FileMeta.Small.Height,
-				Size:   fmt.Sprintf("%dx%d", a.FileMeta.Small.Width, a.FileMeta.Small.Height),
+				Size:   strconv.Itoa(a.FileMeta.Small.Width) + "x" + strconv.Itoa(a.FileMeta.Small.Height),
 				Aspect: float32(a.FileMeta.Small.Aspect),
 			},
-			Focus: apimodel.MediaFocus{
-				X: a.FileMeta.Focus.X,
-				Y: a.FileMeta.Focus.Y,
-			},
 		},
 		Blurhash: a.Blurhash,
 	}
@@ -318,20 +312,31 @@ func (c *converter) AttachmentToAPIAttachment(ctx context.Context, a *gtsmodel.M
 		apiAttachment.Description = &i
 	}
 
-	if i := a.FileMeta.Original.Duration; i != nil {
-		apiAttachment.Meta.Original.Duration = *i
-	}
+	// type specific fields
+	switch a.Type {
+	case gtsmodel.FileTypeImage:
+		apiAttachment.Meta.Original.Size = strconv.Itoa(a.FileMeta.Original.Width) + "x" + strconv.Itoa(a.FileMeta.Original.Height)
+		apiAttachment.Meta.Original.Aspect = float32(a.FileMeta.Original.Aspect)
+		apiAttachment.Meta.Focus = &apimodel.MediaFocus{
+			X: a.FileMeta.Focus.X,
+			Y: a.FileMeta.Focus.Y,
+		}
+	case gtsmodel.FileTypeVideo:
+		if i := a.FileMeta.Original.Duration; i != nil {
+			apiAttachment.Meta.Original.Duration = *i
+		}
 
-	if i := a.FileMeta.Original.Framerate; i != nil {
-		// the masto api expects this as a string in
-		// the format `integer/1`, so 30fps is `30/1`
-		round := math.Round(float64(*i))
-		fr := strconv.FormatInt(int64(round), 10)
-		apiAttachment.Meta.Original.FrameRate = fr + "/1"
-	}
+		if i := a.FileMeta.Original.Framerate; i != nil {
+			// the masto api expects this as a string in
+			// the format `integer/1`, so 30fps is `30/1`
+			round := math.Round(float64(*i))
+			fr := strconv.FormatInt(int64(round), 10)
+			apiAttachment.Meta.Original.FrameRate = fr + "/1"
+		}
 
-	if i := a.FileMeta.Original.Bitrate; i != nil {
-		apiAttachment.Meta.Original.Bitrate = int(*i)
+		if i := a.FileMeta.Original.Bitrate; i != nil {
+			apiAttachment.Meta.Original.Bitrate = int(*i)
+		}
 	}
 
 	return apiAttachment, nil

internal/typeutils/internaltofrontend_test.go

@@ -441,19 +441,13 @@ func (suite *InternalToFrontendTestSuite) TestVideoAttachmentToFrontend() {
       "height": 404,
       "frame_rate": "30/1",
       "duration": 15.033334,
-      "bitrate": 1206522,
-      "size": "720x404",
-      "aspect": 1.7821782
+      "bitrate": 1206522
     },
     "small": {
       "width": 720,
       "height": 404,
       "size": "720x404",
       "aspect": 1.7821782
-    },
-    "focus": {
-      "x": 0,
-      "y": 0
     }
   },
   "description": "A cow adorably licking another cow!"