fix(previews): ffprobe hangs#60916
Conversation
Signed-off-by: Andrey Dyakov <adduxa@gmail.com>
adduxa
requested review from
ArtificialOwl,
artonge,
icewind1991 and
leftybournes
and removed request for
a team
| } | ||
| foreach ($read as $pipe) { | ||
| $chunk = fread($pipe, 8192); | ||
| if ($chunk === false) continue; |
| !feof($test_hdr_pipes[2]) ? $test_hdr_pipes[2] : null, | ||
| ]); | ||
| $write = $except = []; | ||
| if (stream_select($read, $write, $except, 5) === false) { |
There was a problem hiding this comment.
This 5 is a per-iteration timeout. Only the false (error) return is handled; the 0 (timeout) return falls through, $read becomes empty, the foreach does nothing, and the while loops again. So if ffprobe truly hangs (produces no output and never closes its pipes), this spins every 5s forever: the same "hangs indefinitely" symptom the PR title targets, just via a different path.
There was a problem hiding this comment.
Thanks! I will search for the solution.
Do you know if stdout is always <64k here? Is it acceptable to just read stderr first in this case?
This approach is used at https://github.com/adduxa/server/blob/1bcc80b5f45a0d8619df64edbd2fec797fd70cd9/lib/private/Preview/Movie.php#L374 for ffmpeg
There was a problem hiding this comment.
In useHDR, yes, stdout is always <64K here due to the command line switches passed '-of', 'default=noprint_wrappers=1:nokey=1'. The value will always be something like 'bt709' or (in the case of an HDR file) 'smpte2084' or 'arib-std-b67', so very small.
In the case of useHDR I think a better solution (which I am working on) is filtering the output using command line switches from ffprobe since it is all extraneous information that we do nothing with. useHDR only needs to know if libzimg is installed, and if the file is HDR (based on the colortransfer value from ffprobe).
Also, I replied separately in your issue thread with some important questions, and a request for a sample file for testing purposes.
Thanks!
There was a problem hiding this comment.
Thanks, got your file, looked into it a bit. You are spot on! Just flipping the two lines and reading stderr first will resolve this. Preview generation itself already does it in this order. ffmpeg outputs nothing to stdout unless we tell it to like in useHDR (and very little in this case, will never fill the buffer)
There was a problem hiding this comment.
Thank you for you time! I pushed a new fix
Signed-off-by: Andrey Dyakov <adduxa@gmail.com>
|
Maintainers are going to ask you to squash your commits. |
4 participants
Summary
Fix pipe buffer deadlock in
useHdr()inlib/private/Preview/Movie.php.When
ffprobeproduces more than ~64KB on stderr, the sequentialstream_get_contentscallsdeadlock: PHP blocks reading stdout while ffprobe blocks flushing stderr. This caused
occ preview:generate-allto hang indefinitely on certain video files.A tempting workaround is to swap the two
stream_get_contentscalls (reading stderr first),but I'm not sure if ffprobe is guaranteed to produce <64KB on the stdout for this call.
Fix drains both pipes concurrently using
stream_selectwith non-blocking I/O.TODO
Checklist
3. to review, feature component)stable32)AI (if applicable)