Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sipi): upload video support (DEV-771 / DEV-207) #1952

Merged
merged 56 commits into from Apr 5, 2022

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Nov 29, 2021

resolves DEV-771 and DEV-207

@kilchenmann kilchenmann self-assigned this Nov 29, 2021
@subotic subotic marked this pull request as draft December 10, 2021 17:45
@sonarcloud
Copy link

sonarcloud bot commented Dec 14, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kilchenmann kilchenmann self-requested a review April 1, 2022 14:29
@kilchenmann kilchenmann added enhancement improve existing code or new feature Sipi dependencies Pull requests that update a dependency file (with fuseki, sipi, etc.) labels Apr 1, 2022
@kilchenmann kilchenmann marked this pull request as ready for review April 1, 2022 15:41
@kilchenmann kilchenmann changed the title feat(sipi): upload video support (DEV-25) feat(sipi): upload video support (DEV-25 / DEV-207) Apr 4, 2022
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some minor comments, feel free to ignore them ;)

Comment on lines +40 to +41
sipi/images/1111/*
sipi/images/originals/1111/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we shouldn't ignore all things here, and explicitly un-ignore what we want to have?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we define it in a separate task. Because for me it's not clear, which folder should be kept and maybe it has to be discussed.

Comment on lines -1202 to +1200
:subjectClassConstraint :MovingImageFileValue ;
:subjectClassConstraint :FileValue ;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this changed? IMO, fps should only exist on moving images, not generally on files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @subotic changed it. Because we no longer store the necessary moving image metadata such as width, height, duration and fps in the database. We get this information from the sidecar file from sipi.

Comment on lines 145 to 151
# check the outpath for the matrix files;
# if exists, delete it and create new
# if [ -d "matrix" ]
# then
# rm -rf ./matrix
# fi
# mkdir -p 'matrix'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can probably be removed, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 02412c9

end
})

return M
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline at end of file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in bc704e3

Comment on lines +1 to +23
--
-- json.lua
--
-- Copyright (c) 2020 rxi
--
-- Permission is hereby granted, free of charge, to any person obtaining a copy of
-- this software and associated documentation files (the "Software"), to deal in
-- the Software without restriction, including without limitation the rights to
-- use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
-- of the Software, and to permit persons to whom the Software is furnished to do
-- so, subject to the following conditions:
--
-- The above copyright notice and this permission notice shall be included in all
-- copies or substantial portions of the Software.
--
-- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
-- SOFTWARE.
--
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it normal in lua world, that we have to include 3rd party scripts as such in our repo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this json script to handle the sidecar file (convert and read).

sipi/scripts/store.lua Outdated Show resolved Hide resolved
Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some minor details and questions

#!/bin/bash

# Export moving image frames for preview in search results and on timeline
# Author: André Kilchenmann, a.kilchenmann@unibas.ch

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have the author here? In all other scripts we always just used the licence in the top part of the file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to remove it. Resolved in 60f7cec

@@ -0,0 +1,108 @@
local M = {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, we have the licence on top of lua scripts:

-- * Copyright © 2021 - 2022 Swiss National Data and Service Center for the Humanities and/or DaSCH Service Platform contributors.
-- * SPDX-License-Identifier: Apache-2.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this file was not written by us, just downloaded from somewhere. But to be honest, I think it's no longer needed. Maybe a remnant from testing the video upload.

File removed in 0a7dab5

sipi/scripts/upload.lua Outdated Show resolved Hide resolved
Comment on lines 25 to 29
-- Write sidecar file
function write_sidecar_file()


end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know almost nothing about lua but this seems like an empty function. Is it correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 5a7f056

sipi/scripts/upload.lua Outdated Show resolved Hide resolved
fps = fps
}

-- TODO: similar setup for audio files; get duration with ffprobe and write extended sidecar data

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be kept here? Is there a JIRA issue defined for this task or how do we remember to actually do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the question. I've created a new subtask DEV-770

@kilchenmann kilchenmann changed the title feat(sipi): upload video support (DEV-25 / DEV-207) feat(sipi): upload video support (DEV-771 / DEV-207) Apr 5, 2022
@sonarcloud
Copy link

sonarcloud bot commented Apr 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@subotic subotic merged commit 47f2e28 into main Apr 5, 2022
@subotic subotic deleted the wip/dev-25-upload-video-support branch April 5, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file (with fuseki, sipi, etc.) enhancement improve existing code or new feature Sipi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants