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

recode videos incompatible with popular browsers #886

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kabakaev
Copy link
Contributor

@kabakaev kabakaev commented Sep 2, 2023

This PR fixes #606 and partly #387, except a remux of existing video stream.

@bobobo1618, can you give an example of video files worth remuxing?

My video archive has only a handful of records from a digital video camera, which saved H.264 video and AC3 audio in MPEGTS container. But even there, I'd prefer to spend some CPU time to re-encode that hardware-encoded H.264 stream through libx264 to avoid artifacts, improve compression (Main vs. High profile) and to validate the stream.

Another possible use case would be to remux videos in order to move the MP4 moov atom in front of mdat for faster start of video playback. But I guess it's better to prepare such a remux on-the-fly instead of on-disk caching.

Anyone willing to make use of this PR should remove the "original" media_urls and then press Scan button for the affected users.
For example, H.264-encoding of all google pixel videos can be triggered by this SQL statement:

  1. Make backup of the database
  2. delete from media_urls where media_name LIKE 'PXL%.mp4' AND purpose=='original';

A combined code of this PR and #883 is built and pushed to container registry at docker.io/kabakaev/photoview:recode-incompatible-video

@mawi675lr81, @ouafnico, @CorneliousJD, would you like to test your video files with this fixed version (avoid running on production though, or make a backup)?

The combined code is pushed at https://github.com/kabakaev/photoview/tree/recode-incompatible-video

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.18%. Comparing base (8cdc4cd) to head (fed8366).
Report is 15 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #886   +/-   ##
=======================================
  Coverage   36.18%   36.18%           
=======================================
  Files          38       38           
  Lines        1708     1708           
=======================================
  Hits          618      618           
  Misses        934      934           
  Partials      156      156           
Flag Coverage Δ
api 36.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kkovaletp
Copy link
Contributor

@kabakaev Why not to include the mentioned SQL query as the migration code, affecting all original videos? In this case, when this PR is merged, the fix will be automatically applied to all user's envs after the next rescan.

A really small amount of users actually read the "What's new" of a release, so if some manual steps are required for the fix, in most cases that won't be performed

@kabakaev
Copy link
Contributor Author

kabakaev commented Sep 2, 2023

This may be a good idea.
Unfortunately, the given SQL statement is not idempotent. In other words, it can only be run once.
ALso, it should not delete the "original" media_urls, which are already compatible with web browsers, such as MP4 files with H.264 and AAC codecs.

We may call ffprobe for every scanned video file, but it will make the scan job painfully slow. The only viable and performant option would be to extend the media_urls table with videoCodecs, audioCodecs names and container options. If we fill them once, then future improvements may be easier to implement. For example, on-the-fly recode of MP4 container for fast start would be fast enough.
@viktorstrate, how do you think, is it possible to extend the media_urls with those 3 fields?
Alternatively, we may create two new tables: a media_codecs table listing all known codecs and a media_urls_codecs many-to-many table joining media_urls and media_codecs.

@kkovaletp
Copy link
Contributor

@kabakaev As the query is not idempotent, it should be run inside the transaction, so we can be sure that nothing is corrupted in case of some interruption or failure.

As far as I see from the DB structure, there is no need to extend the media_urls table with additional fields or create new tables, as we already have the video_metadata table, whose purpose is to contain such data. Please check whether it is, and if not, we potentially need to enhance or extend the data in this table.

According to the schema, the connection between the media_urls and video_metadata tables can be done through the media table.

select * 
from media_urls mu 
	join media m on mu.media_id = m.id 
	JOIN video_metadata vm on vm.id = m.video_metadata_id 
where mu.purpose = 'original';

Anyway, analyzing potential "damage", I don't see a big problem with removing more "original" media_urls than technically needed: the next scan will add them back spending not so much additional time and resources. It will be definitely faster than running ffprobe for each video file

@kkovaletp
Copy link
Contributor

@kabakaev, I've created PR #910 - a temporary alternative to the master while the project is on hold. I'd like to merge this PR as well, but I don't believe users will run manual steps after pulling new code and rebuilding the image, so I'd ask you to prepare a migration for the DB without an extension for the media_urls table and using existing data and relations, as proposed in my previous comment. This migration should cover all the cases when this PR might be useful (remove videos of all formats that need to be rescanned).
Do we need to remove corresponding files from the cache as well? If original files are going to be rescanned, will that remove previously converted files or they will stay and just waste space?

@viktorstrate
Copy link
Member

The database can be automatically migrated to solve the problem of deleting the old records.

This is done in api/database/database.go

@kkovaletp
Copy link
Contributor

@jordy2254, what do you think about this PR? My questions are in the comment above

@kkovaletp kkovaletp added discussion Raises questions that are up for discussion feature A new idea or feature scanner Related to the scanner component labels Apr 29, 2024
@kkovaletp kkovaletp linked an issue May 12, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Raises questions that are up for discussion feature A new idea or feature scanner Related to the scanner component
Projects
None yet
3 participants