Video file metadata PR cleanup

8 files changed, 102 insertions, 99 deletions

diff --git a/server/helpers/custom-validators/activitypub/videos.ts b/server/helpers/custom-validators/activitypub/videos.ts
index af8c8a0c8..876cc7f50 100644
--- a/server/helpers/custom-validators/activitypub/videos.ts
+++ b/server/helpers/custom-validators/activitypub/videos.ts
@@ -13,6 +13,7 @@ import {
 import { isActivityPubUrlValid, isBaseActivityValid, setValidAttributedTo } from './misc'
 import { VideoState } from '../../../../shared/models/videos'
 import { logger } from '@server/helpers/logger'
+import { ActivityVideoFileMetadataObject } from '@shared/models'
 
 function sanitizeAndCheckVideoTorrentUpdateActivity (activity: any) {
   return isBaseActivityValid(activity, 'Update') &&
@@ -104,7 +105,15 @@ function isRemoteVideoUrlValid (url: any) {
       (url.mediaType || url.mimeType) === 'application/x-mpegURL' &&
       isActivityPubUrlValid(url.href) &&
       isArray(url.tag)
-    )
+    ) ||
+    isAPVideoFileMetadataObject(url)
+}
+
+function isAPVideoFileMetadataObject (url: any): url is ActivityVideoFileMetadataObject {
+  return url &&
+    url.type === 'Link' &&
+    url.mediaType === 'application/json' &&
+    isArray(url.rel) && url.rel.includes('metadata')
 }
 
 // ---------------------------------------------------------------------------
@@ -113,7 +122,8 @@ export {
   sanitizeAndCheckVideoTorrentUpdateActivity,
   isRemoteStringIdentifierValid,
   sanitizeAndCheckVideoTorrentObject,
-  isRemoteVideoUrlValid
+  isRemoteVideoUrlValid,
+  isAPVideoFileMetadataObject
 }
 
 // ---------------------------------------------------------------------------
diff --git a/server/helpers/ffmpeg-utils.ts b/server/helpers/ffmpeg-utils.ts
index 5ee295635..e0e408ea0 100644
--- a/server/helpers/ffmpeg-utils.ts
+++ b/server/helpers/ffmpeg-utils.ts
@@ -170,7 +170,7 @@ async function getVideoFileFPS (path: string) {
   return 0
 }
 
-async function getMetadataFromFile<T> (path: string, cb = metadata => metadata) {
+async function getMetadataFromFile <T> (path: string, cb = metadata => metadata) {
   return new Promise<T>((res, rej) => {
     ffmpeg.ffprobe(path, (err, metadata) => {
       if (err) return rej(err)
diff --git a/server/lib/activitypub/videos.ts b/server/lib/activitypub/videos.ts
index 30de4714c..452e43c8c 100644
--- a/server/lib/activitypub/videos.ts
+++ b/server/lib/activitypub/videos.ts
@@ -9,13 +9,13 @@ import {
   ActivityPlaylistUrlObject,
   ActivityTagObject,
   ActivityUrlObject,
+  ActivityVideoFileMetadataObject,
   ActivityVideoUrlObject,
-  VideoState,
-  ActivityVideoFileMetadataObject
+  VideoState
 } from '../../../shared/index'
 import { VideoTorrentObject } from '../../../shared/models/activitypub/objects'
 import { VideoPrivacy } from '../../../shared/models/videos'
-import { sanitizeAndCheckVideoTorrentObject } from '../../helpers/custom-validators/activitypub/videos'
+import { sanitizeAndCheckVideoTorrentObject, isAPVideoFileMetadataObject } from '../../helpers/custom-validators/activitypub/videos'
 import { isVideoFileInfoHashValid } from '../../helpers/custom-validators/videos'
 import { deleteNonExistingModels, resetSequelizeInstance, retryTransactionWrapper } from '../../helpers/database-utils'
 import { logger } from '../../helpers/logger'
@@ -26,7 +26,8 @@ import {
   P2P_MEDIA_LOADER_PEER_VERSION,
   PREVIEWS_SIZE,
   REMOTE_SCHEME,
-  STATIC_PATHS, THUMBNAILS_SIZE
+  STATIC_PATHS,
+  THUMBNAILS_SIZE
 } from '../../initializers/constants'
 import { TagModel } from '../../models/video/tag'
 import { VideoModel } from '../../models/video/video'
@@ -69,7 +70,8 @@ import {
   MVideoAPWithoutCaption,
   MVideoFile,
   MVideoFullLight,
-  MVideoId, MVideoImmutable,
+  MVideoId,
+  MVideoImmutable,
   MVideoThumbnail
 } from '../../typings/models'
 import { MThumbnail } from '../../typings/models/video/thumbnail'
@@ -527,10 +529,6 @@ function isAPHashTagObject (url: any): url is ActivityHashTagObject {
   return url && url.type === 'Hashtag'
 }
 
-function isAPVideoFileMetadataObject (url: any): url is ActivityVideoFileMetadataObject {
-  return url && url.type === 'Link' && url.mediaType === 'application/json' && url.hasAttribute('rel') && url.rel.includes('metadata')
-}
-
 async function createVideo (videoObject: VideoTorrentObject, channel: MChannelAccountLight, waitThumbnail = false) {
   logger.debug('Adding remote video %s.', videoObject.id)
 
@@ -701,11 +699,11 @@ function videoFileActivityUrlToDBAttributes (
 
     // Fetch associated metadata url, if any
     const metadata = urls.filter(isAPVideoFileMetadataObject)
-                          .find(u =>
-                            u.height === fileUrl.height &&
-                            u.fps === fileUrl.fps &&
-                            u.rel.includes(fileUrl.mediaType)
-                          )
+                         .find(u => {
+                           return u.height === fileUrl.height &&
+                             u.fps === fileUrl.fps &&
+                             u.rel.includes(fileUrl.mediaType)
+                         })
 
     const mediaType = fileUrl.mediaType
     const attribute = {
diff --git a/server/lib/video-transcoding.ts b/server/lib/video-transcoding.ts
index 444b0d954..bde4c2633 100644
--- a/server/lib/video-transcoding.ts
+++ b/server/lib/video-transcoding.ts
@@ -237,12 +237,9 @@ async function onVideoFileTranscoding (video: MVideoWithFile, videoFile: MVideoF
 
   await move(transcodingPath, outputPath)
 
-  const extractedVideo = extractVideo(video)
-
   videoFile.size = stats.size
   videoFile.fps = fps
   videoFile.metadata = metadata
-  videoFile.metadataUrl = extractedVideo.getVideoFileMetadataUrl(videoFile, extractedVideo.getBaseUrls().baseUrlHttp)
 
   await createTorrentAndSetInfoHash(video, videoFile)
 
diff --git a/server/models/video/video-file.ts b/server/models/video/video-file.ts
index 029468004..201f0c0f1 100644
--- a/server/models/video/video-file.ts
+++ b/server/models/video/video-file.ts
@@ -30,18 +30,16 @@ import { MIMETYPES, MEMOIZE_LENGTH, MEMOIZE_TTL } from '../../initializers/const
 import { MVideoFile, MVideoFileStreamingPlaylistVideo, MVideoFileVideo } from '../../typings/models/video/video-file'
 import { MStreamingPlaylistVideo, MVideo } from '@server/typings/models'
 import * as memoizee from 'memoizee'
+import validator from 'validator'
 
 export enum ScopeNames {
   WITH_VIDEO = 'WITH_VIDEO',
-  WITH_VIDEO_OR_PLAYLIST = 'WITH_VIDEO_OR_PLAYLIST',
   WITH_METADATA = 'WITH_METADATA'
 }
 
-const METADATA_FIELDS = [ 'metadata', 'metadataUrl' ]
-
 @DefaultScope(() => ({
   attributes: {
-    exclude: [ METADATA_FIELDS[0] ]
+    exclude: [ 'metadata' ]
   }
 }))
 @Scopes(() => ({
@@ -53,35 +51,9 @@ const METADATA_FIELDS = [ 'metadata', 'metadataUrl' ]
       }
     ]
   },
-  [ScopeNames.WITH_VIDEO_OR_PLAYLIST]: (videoIdOrUUID: string | number) => {
-    const where = (typeof videoIdOrUUID === 'number')
-      ? { id: videoIdOrUUID }
-      : { uuid: videoIdOrUUID }
-
-    return {
-      include: [
-        {
-          model: VideoModel.unscoped(),
-          required: false,
-          where
-        },
-        {
-          model: VideoStreamingPlaylistModel.unscoped(),
-          required: false,
-          include: [
-            {
-              model: VideoModel.unscoped(),
-              required: true,
-              where
-            }
-          ]
-        }
-      ]
-    }
-  },
   [ScopeNames.WITH_METADATA]: {
     attributes: {
-      include: METADATA_FIELDS
+      include: [ 'metadata' ]
     }
   }
 }))
@@ -223,10 +195,8 @@ export class VideoFileModel extends Model<VideoFileModel> {
 
   static async doesVideoExistForVideoFile (id: number, videoIdOrUUID: number | string) {
     const videoFile = await VideoFileModel.loadWithVideoOrPlaylist(id, videoIdOrUUID)
-    return (videoFile?.Video.id === videoIdOrUUID) ||
-           (videoFile?.Video.uuid === videoIdOrUUID) ||
-           (videoFile?.VideoStreamingPlaylist?.Video?.id === videoIdOrUUID) ||
-           (videoFile?.VideoStreamingPlaylist?.Video?.uuid === videoIdOrUUID)
+
+    return !!videoFile
   }
 
   static loadWithMetadata (id: number) {
@@ -238,12 +208,41 @@ export class VideoFileModel extends Model<VideoFileModel> {
   }
 
   static loadWithVideoOrPlaylist (id: number, videoIdOrUUID: number | string) {
-    return VideoFileModel.scope({
-      method: [
-        ScopeNames.WITH_VIDEO_OR_PLAYLIST,
-        videoIdOrUUID
+    const whereVideo = validator.isUUID(videoIdOrUUID + '')
+      ? { uuid: videoIdOrUUID }
+      : { id: videoIdOrUUID }
+
+    const options = {
+      where: {
+        id
+      },
+      include: [
+        {
+          model: VideoModel.unscoped(),
+          required: false,
+          where: whereVideo
+        },
+        {
+          model: VideoStreamingPlaylistModel.unscoped(),
+          required: false,
+          include: [
+            {
+              model: VideoModel.unscoped(),
+              required: true,
+              where: whereVideo
+            }
+          ]
+        }
       ]
-    }).findByPk(id)
+    }
+
+    return VideoFileModel.findOne(options)
+      .then(file => {
+        // We used `required: false` so check we have at least a video or a streaming playlist
+        if (!file.Video && !file.VideoStreamingPlaylist) return null
+
+        return file
+      })
   }
 
   static listByStreamingPlaylist (streamingPlaylistId: number, transaction: Transaction) {
diff --git a/server/models/video/video-format-utils.ts b/server/models/video/video-format-utils.ts
index 21f0e0a68..365c9581e 100644
--- a/server/models/video/video-format-utils.ts
+++ b/server/models/video/video-format-utils.ts
@@ -181,6 +181,8 @@ function videoFilesModelToFormattedJSON (
   baseUrlWs: string,
   videoFiles: MVideoFileRedundanciesOpt[]
 ): VideoFile[] {
+  const video = extractVideo(model)
+
   return videoFiles
     .map(videoFile => {
       return {
@@ -195,7 +197,7 @@ function videoFilesModelToFormattedJSON (
         torrentDownloadUrl: model.getTorrentDownloadUrl(videoFile, baseUrlHttp),
         fileUrl: model.getVideoFileUrl(videoFile, baseUrlHttp),
         fileDownloadUrl: model.getVideoFileDownloadUrl(videoFile, baseUrlHttp),
-        metadataUrl: videoFile.metadataUrl // only send the metadataUrl and not the metadata over the wire
+        metadataUrl: video.getVideoFileMetadataUrl(videoFile, baseUrlHttp)
       } as VideoFile
     })
     .sort((a, b) => {
diff --git a/server/models/video/video.ts b/server/models/video/video.ts
index 5e4b7d44c..958a49e65 100644
--- a/server/models/video/video.ts
+++ b/server/models/video/video.ts
@@ -1849,7 +1849,8 @@ export class VideoModel extends Model<VideoModel> {
 
   getVideoFileMetadataUrl (videoFile: MVideoFile, baseUrlHttp: string) {
     const path = '/api/v1/videos/'
-    return videoFile.metadata
+
+    return this.isOwned()
       ? baseUrlHttp + path + this.uuid + '/metadata/' + videoFile.id
       : videoFile.metadataUrl
   }
diff --git a/server/tests/api/videos/video-transcoder.ts b/server/tests/api/videos/video-transcoder.ts
index ce0dd14d5..13b3530b1 100644
--- a/server/tests/api/videos/video-transcoder.ts
+++ b/server/tests/api/videos/video-transcoder.ts
@@ -27,13 +27,14 @@ import {
   root,
   ServerInfo,
   setAccessTokensToServers,
-  uploadVideo,
+  uploadVideo, uploadVideoAndGetId,
   waitJobs,
   webtorrentAdd
 } from '../../../../shared/extra-utils'
 import { join } from 'path'
 import { VIDEO_TRANSCODING_FPS } from '../../../../server/initializers/constants'
 import { FfprobeData } from 'fluent-ffmpeg'
+import { VideoFileMetadata } from '@shared/models/videos/video-file-metadata'
 
 const expect = chai.expect
 
@@ -470,61 +471,56 @@ describe('Test video transcoding', function () {
   it('Should provide valid ffprobe data', async function () {
     this.timeout(160000)
 
-    const videoAttributes = {
-      name: 'my super name for server 1',
-      description: 'my super description for server 1',
-      fixture: 'video_short.webm'
-    }
-    await uploadVideo(servers[1].url, servers[1].accessToken, videoAttributes)
-
+    const videoUUID = (await uploadVideoAndGetId({ server: servers[1], videoName: 'ffprobe data' })).uuid
     await waitJobs(servers)
 
-    const res = await getVideosList(servers[1].url)
+    {
+      const path = join(root(), 'test' + servers[1].internalServerNumber, 'videos', videoUUID + '-240.mp4')
+      const metadata = await getMetadataFromFile<VideoFileMetadata>(path)
 
-    const videoOnOrigin = res.body.data.find(v => v.name === videoAttributes.name)
-    const res2 = await getVideo(servers[1].url, videoOnOrigin.id)
-    const videoOnOriginDetails: VideoDetails = res2.body
+      // expected format properties
+      for (const p of [
+        'tags.encoder',
+        'format_long_name',
+        'size',
+        'bit_rate'
+      ]) {
+        expect(metadata.format).to.have.nested.property(p)
+      }
 
-    {
-      const path = join(root(), 'test' + servers[1].internalServerNumber, 'videos', videoOnOrigin.uuid + '-240.mp4')
-      const metadata = await getMetadataFromFile(path)
+      // expected stream properties
       for (const p of [
-        // expected format properties
-        'format.encoder',
-        'format.format_long_name',
-        'format.size',
-        'format.bit_rate',
-        // expected stream properties
-        'stream[0].codec_long_name',
-        'stream[0].profile',
-        'stream[0].width',
-        'stream[0].height',
-        'stream[0].display_aspect_ratio',
-        'stream[0].avg_frame_rate',
-        'stream[0].pix_fmt'
+        'codec_long_name',
+        'profile',
+        'width',
+        'height',
+        'display_aspect_ratio',
+        'avg_frame_rate',
+        'pix_fmt'
       ]) {
-        expect(metadata).to.have.nested.property(p)
+        expect(metadata.streams[0]).to.have.nested.property(p)
       }
+
       expect(metadata).to.not.have.nested.property('format.filename')
     }
 
     for (const server of servers) {
-      const res = await getVideosList(server.url)
-
-      const video = res.body.data.find(v => v.name === videoAttributes.name)
-      const res2 = await getVideo(server.url, video.id)
-      const videoDetails = res2.body
+      const res2 = await getVideo(server.url, videoUUID)
+      const videoDetails: VideoDetails = res2.body
 
       const videoFiles = videoDetails.files
-      for (const [ index, file ] of videoFiles.entries()) {
+                                     .concat(videoDetails.streamingPlaylists[0].files)
+      expect(videoFiles).to.have.lengthOf(8)
+
+      for (const file of videoFiles) {
         expect(file.metadata).to.be.undefined
+        expect(file.metadataUrl).to.exist
         expect(file.metadataUrl).to.contain(servers[1].url)
-        expect(file.metadataUrl).to.contain(videoOnOrigin.uuid)
+        expect(file.metadataUrl).to.contain(videoUUID)
 
         const res3 = await getVideoFileMetadataUrl(file.metadataUrl)
         const metadata: FfprobeData = res3.body
         expect(metadata).to.have.nested.property('format.size')
-        expect(metadata.format.size).to.equal(videoOnOriginDetails.files[index].metadata.format.size)
       }
     }
   })