From 450abaff2ed4053e009a92ed3bab42b9301b0668 Mon Sep 17 00:00:00 2001 From: Max Holland Date: Tue, 13 Aug 2024 15:23:01 +0100 Subject: [PATCH] Fix thumbs for recording fallback (#1359) * Fix thumbs for recording fallback * comments --- clients/input_copy.go | 8 ++--- clients/manifest.go | 28 ++++++++++----- clients/manifest_test.go | 76 +++++++++++++++++++++++----------------- thumbnails/thumbnails.go | 8 +++-- 4 files changed, 73 insertions(+), 47 deletions(-) diff --git a/clients/input_copy.go b/clients/input_copy.go index 76956c18f..182752e84 100644 --- a/clients/input_copy.go +++ b/clients/input_copy.go @@ -64,14 +64,14 @@ func (s *InputCopy) CopyInputToS3(requestID string, inputFile, osTransferURL *ur } } - log.Log(requestID, "starting probe", "source", inputFile.String(), "dest", osTransferURL.String()) + log.Log(requestID, "starting probe", "source", inputFile.Redacted(), "dest", osTransferURL.Redacted()) inputFileProbe, err := s.Probe.ProbeFile(requestID, signedURL, "-analyzeduration", "15000000") if err != nil { - log.Log(requestID, "probe failed", "err", err, "source", inputFile.String(), "dest", osTransferURL.String()) + log.Log(requestID, "probe failed", "err", err, "source", inputFile.Redacted(), "dest", osTransferURL.Redacted()) return video.InputVideo{}, "", fmt.Errorf("error probing MP4 input file from S3: %w", err) } - log.Log(requestID, "probe succeeded", "source", inputFile.String(), "dest", osTransferURL.String()) + log.Log(requestID, "probe succeeded", "source", inputFile.Redacted(), "dest", osTransferURL.Redacted()) videoTrack, err := inputFileProbe.GetTrack(video.TrackTypeVideo) hasVideoTrack := err == nil // verify the duration of the video track and don't process if we can't determine duration @@ -229,7 +229,7 @@ func CopyAllInputFiles(requestID string, srcInputUrl, dstOutputUrl *url.URL, dec } byteCount += size } - log.Log(requestID, "Copied", "bytes", byteCount, "source", srcInputUrl.String(), "dest", dstOutputUrl.String()) + log.Log(requestID, "Copied", "bytes", byteCount, "source", srcInputUrl.Redacted(), "dest", dstOutputUrl.Redacted()) return nil } diff --git a/clients/manifest.go b/clients/manifest.go index 8337d8044..e84e76cf0 100644 --- a/clients/manifest.go +++ b/clients/manifest.go @@ -53,10 +53,13 @@ func RecordingBackupCheck(requestID string, primaryManifestURL, osTransferURL *u return primaryManifestURL, nil } - playlist, playlistType, err := downloadManifestWithBackup(requestID, primaryManifestURL.String()) + playlistURL, playlist, playlistType, err := downloadManifestWithBackup(requestID, primaryManifestURL.String()) if err != nil { return nil, fmt.Errorf("error downloading manifest: %w", err) } + // if we had to use the backup location for the manifest then we need to write a new playlist + newPlaylistRequired := playlistURL != primaryManifestURL.String() + mediaPlaylist, err := convertToMediaPlaylist(playlist, playlistType) if err != nil { return nil, err @@ -81,10 +84,19 @@ func RecordingBackupCheck(requestID string, primaryManifestURL, osTransferURL *u if err != nil { return nil, fmt.Errorf("failed to find segment file %s: %w", segURL.Redacted(), err) } + if actualSegURL != segURL.String() { + // if we had to use the backup location for any segment then we need a new manifest file with new segment URLs + // pointing to wherever they are found, primary or backup + newPlaylistRequired = true + } segment.URI = actualSegURL } - // write the manifest to storage and update the manifestURL variable + if !newPlaylistRequired { + return primaryManifestURL, nil + } + + // write the updated manifest to storage and update the manifestURL variable outputStorageURL := osTransferURL.JoinPath("input.m3u8") err = backoff.Retry(func() error { return UploadToOSURL(outputStorageURL.String(), "", strings.NewReader(mediaPlaylist.String()), ManifestUploadTimeout) @@ -118,7 +130,7 @@ func convertToMediaPlaylist(playlist m3u8.Playlist, playlistType m3u8.ListType) return *mediaPlaylist, nil } -func downloadManifestWithBackup(requestID, sourceManifestOSURL string) (m3u8.Playlist, m3u8.ListType, error) { +func downloadManifestWithBackup(requestID, sourceManifestOSURL string) (string, m3u8.Playlist, m3u8.ListType, error) { var playlist, playlistBackup m3u8.Playlist var playlistType, playlistTypeBackup m3u8.ListType var size, sizeBackup int @@ -146,21 +158,21 @@ func downloadManifestWithBackup(requestID, sourceManifestOSURL string) (m3u8.Pla // (only not found errors passthrough below) primaryNotFound, backupNotFound := errors.IsObjectNotFound(errPrimary), errors.IsObjectNotFound(errBackup) if primaryNotFound && backupNotFound { - return nil, 0, errPrimary + return "", nil, 0, errPrimary } if errPrimary != nil && !primaryNotFound { - return nil, 0, errPrimary + return "", nil, 0, errPrimary } if errBackup != nil && !backupNotFound { - return nil, 0, errBackup + return "", nil, 0, errBackup } // Return the largest manifest as the most recent version hasBackup := backupManifestURL != "" && errBackup == nil if hasBackup && (errPrimary != nil || sizeBackup > size) { - return playlistBackup, playlistTypeBackup, nil + return backupManifestURL, playlistBackup, playlistTypeBackup, nil } - return playlist, playlistType, errPrimary + return sourceManifestOSURL, playlist, playlistType, errPrimary } func downloadManifest(requestID, sourceManifestOSURL string) (playlist m3u8.Playlist, playlistType m3u8.ListType, size int, err error) { diff --git a/clients/manifest_test.go b/clients/manifest_test.go index 2ece64ecf..c06e54cf5 100644 --- a/clients/manifest_test.go +++ b/clients/manifest_test.go @@ -370,49 +370,56 @@ seg-1.ts ` tests := []struct { - name string - primaryManifest string - backupManifest string - primarySegments []string - backupSegments []string + name string + primaryManifest string + backupManifest string + primarySegments []string + backupSegments []string + expectedURLChange bool }{ { - name: "happy. all segments and manifest available on primary", - primaryManifest: completeManifest, - backupManifest: "", - primarySegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + name: "happy. all segments and manifest available on primary", + primaryManifest: completeManifest, + backupManifest: "", + primarySegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + expectedURLChange: false, }, { - name: "all segments and manifest available on backup", - primaryManifest: inCompleteManifest, - backupManifest: completeManifest, - backupSegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + name: "all segments and manifest available on backup", + primaryManifest: inCompleteManifest, + backupManifest: completeManifest, + backupSegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + expectedURLChange: true, }, { - name: "all segments on backup and newest manifest on primary", - primaryManifest: completeManifest, - backupManifest: inCompleteManifest, - backupSegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + name: "all segments on backup and newest manifest on primary", + primaryManifest: completeManifest, + backupManifest: inCompleteManifest, + backupSegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + expectedURLChange: true, }, { - name: "all segments on primary and newest manifest on backup", - primaryManifest: inCompleteManifest, - backupManifest: completeManifest, - primarySegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + name: "all segments on primary and newest manifest on backup", + primaryManifest: inCompleteManifest, + backupManifest: completeManifest, + primarySegments: []string{"seg-0.ts", "seg-1.ts", "seg-2.ts", "seg-3.ts"}, + expectedURLChange: true, }, { - name: "segments split between primary and backup, newest manifest on primary", - primaryManifest: completeManifest, - backupManifest: inCompleteManifest, - primarySegments: []string{"seg-0.ts", "seg-2.ts"}, - backupSegments: []string{"seg-1.ts", "seg-3.ts"}, + name: "segments split between primary and backup, newest manifest on primary", + primaryManifest: completeManifest, + backupManifest: inCompleteManifest, + primarySegments: []string{"seg-0.ts", "seg-2.ts"}, + backupSegments: []string{"seg-1.ts", "seg-3.ts"}, + expectedURLChange: true, }, { - name: "segments split between primary and backup, newest manifest on backup", - primaryManifest: inCompleteManifest, - backupManifest: completeManifest, - primarySegments: []string{"seg-0.ts", "seg-2.ts"}, - backupSegments: []string{"seg-1.ts", "seg-3.ts"}, + name: "segments split between primary and backup, newest manifest on backup", + primaryManifest: inCompleteManifest, + backupManifest: completeManifest, + primarySegments: []string{"seg-0.ts", "seg-2.ts"}, + backupSegments: []string{"seg-1.ts", "seg-3.ts"}, + expectedURLChange: true, }, } for _, tt := range tests { @@ -443,8 +450,10 @@ seg-1.ts require.NoError(t, err) } - renditionUrl, err := RecordingBackupCheck("requestID", toUrl(t, filepath.Join(dir, "primary", "index.m3u8")), toUrl(t, filepath.Join(dir, "transfer"))) + primary := toUrl(t, filepath.Join(dir, "primary", "index.m3u8")) + renditionUrl, err := RecordingBackupCheck("requestID", primary, toUrl(t, filepath.Join(dir, "transfer"))) require.NoError(t, err) + require.Equal(t, tt.expectedURLChange, renditionUrl != primary) file, err := os.Open(renditionUrl.String()) require.NoError(t, err) @@ -458,7 +467,8 @@ seg-1.ts require.Len(t, mediaPlaylist.GetAllSegments(), 4) for i, segment := range mediaPlaylist.GetAllSegments() { - require.True(t, filepath.IsAbs(segment.URI)) + // If the URL has been updated then it means we should have a new playlist with absolute URLs + require.Equal(t, filepath.IsAbs(segment.URI), tt.expectedURLChange, "Expected absolute URL? %t: %s", tt.expectedURLChange, segment.URI) require.True(t, true, strings.HasSuffix(segment.URI, fmt.Sprintf("seg-%d.ts", i))) } }) diff --git a/thumbnails/thumbnails.go b/thumbnails/thumbnails.go index f49d952de..2bceb12ed 100644 --- a/thumbnails/thumbnails.go +++ b/thumbnails/thumbnails.go @@ -193,7 +193,11 @@ func GenerateThumbsFromManifest(requestID, input string, output *url.URL) error for _, segment := range mediaPlaylist.GetAllSegments() { segment := segment uploadGroup.Go(func() error { - segURL := inputURL.JoinPath("..", segment.URI) + segURL, _ := url.Parse(segment.URI) + // if the URL is valid and absolute then we should just use it as is, otherwise append the path to inputURL + if segURL == nil || !segURL.IsAbs() { + segURL = inputURL.JoinPath("..", segment.URI) + } var ( rc io.ReadCloser err error @@ -204,7 +208,7 @@ func GenerateThumbsFromManifest(requestID, input string, output *url.URL) error return err }, clients.DownloadRetryBackoff()) if err != nil { - return fmt.Errorf("error downloading manifest: %w", err) + return fmt.Errorf("error downloading segment %s: %w", segURL.Redacted(), err) } defer rc.Close()