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 Dec 10, 2021
@sonarcloud
Copy link

@sonarcloud 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 Apr 1, 2022
@kilchenmann kilchenmann added enhancement Sipi dependencies labels Apr 1, 2022
@kilchenmann kilchenmann marked this pull request as ready for review Apr 1, 2022
@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

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

sipi/images/1111/*
sipi/images/originals/1111/*
Copy link
Collaborator

@BalduinLandolt BalduinLandolt Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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.

:subjectClassConstraint :MovingImageFileValue ;
:subjectClassConstraint :FileValue ;
Copy link
Collaborator

@BalduinLandolt BalduinLandolt Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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.

# 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

@BalduinLandolt BalduinLandolt Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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

@BalduinLandolt BalduinLandolt Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

Choose a reason for hiding this comment

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

Resolved in bc704e3

--
-- 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

@BalduinLandolt BalduinLandolt Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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
Collaborator

@irinaschubert irinaschubert left a comment

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
Copy link
Collaborator

@irinaschubert irinaschubert Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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 = {}
Copy link
Collaborator

@irinaschubert irinaschubert Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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
-- Write sidecar file
function write_sidecar_file()


end
Copy link
Collaborator

@irinaschubert irinaschubert Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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
Copy link
Collaborator

@irinaschubert irinaschubert Apr 4, 2022

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
Collaborator

@kilchenmann kilchenmann Apr 5, 2022

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 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
11 checks passed
@subotic subotic deleted the wip/dev-25-upload-video-support branch Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies enhancement Sipi
Development

Successfully merging this pull request may close these issues.

None yet

4 participants