Lychee
Lychee copied to clipboard
[Enhancement] Size computation rather than command line I/O
After the merge of #1239 It will be possible to know all the sizes stored in the database.
As a result, it would a great to update the code in DiskUsage
to use those values rather than the I/O intensive computing currently done.
https://github.com/LycheeOrg/Lychee/blob/master/app/Metadata/DiskUsage.php#L139
While I really like that idea, I just stumbled across a minor issue. We don't have the file size of the video part for live/motion pictures in the database.
@Chostakovitch ? :)
Sorry, it's been too long and I cannot even see the following of the conversation on Gitter.
The simplest way is to do I/O for the video part because I suspect that most of users do not have their instance full of live photos (?). I think that storing the information in the DB would be too specific and introduce confusion.
Do you agree @nagmat84 ?
See here: https://gitter.im/LycheeOrg/Lobby?at=6272553714df4e44f20bd365 and the subsequent messages.
The idea is to treat the video part as the 8th size variant and store it in size_variants
. This has the following benefits:
- Fast size computation: All we need is a simple
SELECT SUM(filesize) FROM size_variants
and we are done. - Smaller
photos
table: We remove columns from thephotos
table which are probably mostlynull
for the majority of users. This does not only save storage space (something which is rather negligible) but also makes hydration of photo models faster - Cleaner architecture: Parts of Lychee already consider the live photo as a size variant. For example, if one archives a photo one can select one of the usual size variants (thumb, ..., big) or live photo. Storing the live photo as a size variant in the DB does not only make this consistent, but also helps avoiding some ugly if-then-else-branches in the code which are currently necessary, because the live photo is always an edge case.
Thanks for the highlights, seems very elegant to me ! :) I'll probably work on it in a few weeks if it still has to be done.
@Chostakovitch This is a follow-up of the recent discussion in the Gitter Lobby. This are some points of what needs to be done. Attention: The order is not in the correct temporal order, i.e. migration of values obviously needs to be done, before the old values are deleted.
- Table
size_variants
- Additional column
checksum
ofcharacter varying(40) not null
.- This column stores the checksum of every size variant and file
- In order to keep the migration efficient, we should fill the column with the value
0000000000000000000000000000000000000000
for the existing size variants to indicate that the column is not set
- Column
type
: RangeORIGINAL (0)
throughTHUMB (6)
need to be extended byLIVE (7)
.
- Additional column
- Table
photos
- Column
checksum
: needs migration tosize_variants.checksum
for the original type, i.e. wheresize_variants.type == 0
- Column
live_photo_checksum
: needs migration tosize_variants.checksum
for the live photo type, i.e. wheresize_variants.type == 7
- Column
live_photo_short_path
: needs migration tosize_variants.short_path
for the live photo type, , i.e. wheresize_variants.type == 7
- Column
live_photo_content_id
should be renamed tocontent_id
(This has always bothering me, but now it its the time to do it in order to avoid confusion)
- Column
-
GoogleMotionPictureHandler
(seeapp/Image/GoogleMotionPictureHandler
)- Handles the extraction of the video stream using
FFMpeg
, see https://github.com/LycheeOrg/Lychee/blob/1a31f3c451907dcbea72ba39ac96ad6c1b390b3b/app/Image/GoogleMotionPictureHandler.php#L77-L83 - Add a method to
GoogleMotionPictureHandler::getDimension()
which returns anImageDimension
similar toImageHandler::getDimension()
-
FFmpeg
provides methods to query an opened video stream, seeFFMpeg/FFProbe/DataMapping/Stream::getDimensions()
- --> allows to store correct width/height in the size variant
-
- Handles the extraction of the video stream using
-
AddStandaloneStrategy::do()
needs tweaking here https://github.com/LycheeOrg/Lychee/blob/1a31f3c451907dcbea72ba39ac96ad6c1b390b3b/app/Actions/Photo/Strategies/AddStandaloneStrategy.php#L134-L149- Obviously, the
live_photo_short_path
needs not to be stored inphoto
, but in the correctsize_variant
- The line
$videoTargetFile->write($tmpVideoFile->read());
should be$streamStat = $videoTargetFile->write($tmpVideoFile->read(), true);
--> this gives you aStreamStat
with a SHA1 checksum and the file size (hurrah, hurrah) to be stored insize_variant.checksum
andsize_variant.filesize
resp.
- Obviously, the
-
AddVideoPartnerStrategy::do
needs tweaking in a similar fashion -
SizeVariants::create
https://github.com/LycheeOrg/Lychee/blob/1a31f3c451907dcbea72ba39ac96ad6c1b390b3b/app/Models/Extensions/SizeVariants.php#L197 does not only need a parameter$filesize
but also an additional$checksum
or even better: useStreamStat
to pass both values in a single parameter -
SizeVariantDefaultFactory::createSizeVariantInternal
https://github.com/LycheeOrg/Lychee/blob/1a31f3c451907dcbea72ba39ac96ad6c1b390b3b/app/Image/SizeVariantDefaultFactory.php#L185-L192 needs tweaking,save
must be called withtrue
as the 2nd parameter to get aStreamStat
for the saved size variant, thisStreamStat
then needs to be passed tosize_variants->create
instead of only the filesize - Model
Photos
- Remove attribute
checksum
- Remove attribute
live_photo_short_path
- Remove attribute
live_photo_checksum
- Rename
live_photo_content_id
tocontent_id
- Remove attribute
- Model
SizeVariant
: Add attributechecksum
This should be the most crucial steps. I haven't written up all the changes which are necessary to the rest of the code which assumes that the live photo is stored directly in the photo instead of a size variant. But this should rather be straight forward.
@nagmat84 Thanks, a lot, for taking the time to write this. Very helpful. I am unavailable for all august, and not much in september. If it is not urgent, and that nobody started to work on it by the time, I am still in!