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

Allow homarus to use faststart for video conversion #162

Conversation

bibliophileaxe
Copy link
Contributor

@bibliophileaxe bibliophileaxe commented Sep 8, 2022

GitHub Issue: (link)
Islandora/documentation#2162

What does this Pull Request do?

Replaces the moveflags frag_keyframe+empty_moov with faststart

A brief description of what the intended result of the PR will be and/or what problem it solves.
It allows videos to be played instantly on all browsers. On chrome large fragmented videos take too long to play.

What's new?

  • Remove moveflags frag_keyframe+empty_moov
  • Added moveflags faststart
  • Converted file is saved as a tempfile - It is not possible to stream output with faststart.
  • The response has changed from StreamResponse to BinaryFileResponse

How should this be tested?

  • Ingest a large video file preferably mov and wait for derivative generation. Now try to play this file on chrome. There are multiple 206 partial content network requests and video starts playing after the moov atom for each fragment has been loaded.
  • Now try the same with this PR. The video will start playing instantly.

Additional Notes:

Open to suggestions about alternate solutions but I've searched everywhere and couldn't find anything else that works.

Interested parties

@Islandora/committers @Islandora/8-x-committers @adam-vessey @lutaylor @willtp87 @jordandukart

@bibliophileaxe bibliophileaxe force-pushed the 2162-allow-homarus-to-use-faststart-while-converting-videos branch 2 times, most recently from 6754f48 to ec118e4 Compare September 8, 2022 10:16
Copy link
Member

@jordandukart jordandukart left a comment

Choose a reason for hiding this comment

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

Some comments on the PR, in general as well the existing set of tests are failing due to the changes here.

Homarus/src/Controller/HomarusController.php Show resolved Hide resolved
Homarus/src/Controller/HomarusController.php Show resolved Hide resolved
Homarus/src/Controller/HomarusController.php Show resolved Hide resolved
@willtp87
Copy link
Member

During the tech call there was agreement in principal to continue with this approach.

@ruebot
Copy link
Member

ruebot commented Sep 22, 2022

Does this need a separate PR to port this over to 3.x?

@whikloj
Copy link
Member

whikloj commented Sep 22, 2022

I think the plan is to move all the ffmpeg arguments to the islandora context like the other microservices. Just need to figure out an nice way to get them into people's context configs

@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Base: 76.60% // Head: 73.05% // Decreases project coverage by -3.54% ⚠️

Coverage data is based on head (c3c345e) compared to base (277f48a).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##                2.x     #162      +/-   ##
============================================
- Coverage     76.60%   73.05%   -3.55%     
+ Complexity      161      150      -11     
============================================
  Files             6        5       -1     
  Lines           654      605      -49     
============================================
- Hits            501      442      -59     
- Misses          153      163      +10     
Impacted Files Coverage Δ
...d_dir/Homarus/src/Controller/HomarusController.php 84.84% <0.00%> (-15.16%) ⬇️
...d_dir/Houdini/src/Controller/HoudiniController.php

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nchiasson-dgi
Copy link

Ran tests against this PR using a large video file and it appears to perform as advertised, video file could be run immediately upon loading content display page.

@rosiel
Copy link
Member

rosiel commented Oct 12, 2022

Discussion from the Tech Call:
Merging, but... as @ruebot said we should port this to 3.x, as that's where new development should be. (Hopefully it isn't too difficult with the move of Homarus to Symfony!) Per @whikloj 's comment, If we want to move all options into config within the Drupal Actions, that's great but shouldn't block this.

@rosiel rosiel merged commit 48cd6ac into Islandora:2.x Oct 12, 2022
@ruebot
Copy link
Member

ruebot commented Oct 12, 2022

I cribbed from this PR and have it over here in our fork of 3.x. There is .mov stuff in there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants