Recorder upgrades (queue, FLAC, fonts, range sort)#8877
Merged
jeeb merged 5 commits intompv-player:masterfrom Jul 8, 2021
Merged
Recorder upgrades (queue, FLAC, fonts, range sort)#8877jeeb merged 5 commits intompv-player:masterfrom
jeeb merged 5 commits intompv-player:masterfrom
Conversation
In commit f767857, wm4 chooses to mux all remaining packets when mp_recorder_mark_discontinuity() is called and adds a call to mux_packets(). However, it is called only after flush_packets(), which clears the packets before they can be muxed out. This has no ill effects per se - recordings end on keyframes, as before - but judging from his commit message, the intention explicitly was to output the inter frames, since long GOPs can mean several seconds of missing content from the output. So, clear the stream packet queues only after the final mux. Also, flushing can mean both discarding and committing. What a country!
Sagnac
added a commit
to Sagnac/streamsave
that referenced
this pull request
Jun 4, 2021
Document new force_extension option. Revise the Known Issues section in accordance with the codec_tag fix in mpv-player/mpv@643c699 and the upcoming packet queue fix etc. in mpv-player/mpv#8877
sfan5
reviewed
Jun 7, 2021
Contributor
Author
sfan5
reviewed
Jun 12, 2021
TheAMM
force-pushed
the
recorder-upgrades
branch
from
8821dcc to
c14892c
Compare
dump_cache() calls qsort() to order an array of pointers, while the comparator forgets it's receiving pointers to pointers. Since cache-dumping over multiple cache ranges is fairly rare, this seems to have gone unnoticed.
TheAMM
force-pushed
the
recorder-upgrades
branch
from
c14892c to
8b7eb73
Compare
Contributor
Author
|
Force-pushed the updated range sort commit as discussed on IRC. |
Contributor
Author
For muxing, FFmpeg expects the FLAC extradata to be just the bare STREAMINFO, and passing the full FLAC extradata (fLaC header and block size, with any additional channel layout metadata) will result in malformed output, as ffmpeg will simply prefix another fLaC header in front. This can be considered to be a bug. FFmpeg's own demuxers only store the STREAMINFO, hence the naivety, while our common source of FLAC streams, the matroska demuxer, holds onto the full extradata. It has been deemed preferable to adjust the extradata upon muxing, instead of in the demuxer (ffmpeg's FLAC decoder knows to read the full fLaC extradata). This fixes muxing FLAC streams, meaning recorder.c or dump-cache.
I've looked and studied the flow in the recorder, and to my understanding, the packet queue is moot after the initial sync, maybe even then - but that's beyond me right now. With the previous choice to mux trailing packets whatever the case, this doesn't result in any new ill effects (and some missing packets at the end is no big deal). Notably, since we don't have to hold onto the packets after we get muxing, we'll never run into any issues with veeery long GOPs filling up our queue (resulting in dropped packets and much user chagrin).
Though, only when the output format is matroska, to avoid muxing errors. This is quite useful when the input has ASS subtitles, as they tend to rely on embedded fonts.
jeeb
approved these changes
Jul 8, 2021
Member
There was a problem hiding this comment.
LGTM.
The reason for doing the extradata filtering on the muxing side of things is because the interface between a demuxer and the following component for decoder initialization data AKA extradata (usually decoder, but can be other things like the recorder) is the decoder initialization data specified in the container (just like avc or hevc or av1 configuration record is passed on to decoders).
I have only taken a limited look at the packet queue removal bits, but since @sfan5 has noted that he has taken a look at that, I am OK with that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here's a bunch of goodies for the recorder feature (
dump-cachethat is,--record-filecan rot but does gain here too).Includes fixes for some slight human errors, but also support for cache dumping FLAC streams (previously broken), streams with very long GOPs (also previously broken) and attaching embedded fonts when outputting to matroska (quite useful with ASS subtitles).
The packet queue (long GOPs) commit fixes #8278.