-
-
Notifications
You must be signed in to change notification settings - Fork 357
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@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 |
This may be a good idea. We may call |
@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 According to the schema, the connection between the 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 |
@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 |
The database can be automatically migrated to solve the problem of deleting the old records. This is done in |
@jordy2254, what do you think about this PR? My questions are in the comment above |
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 ofmdat
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:
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