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

video_core, citra_qt: Video dumping updates #5083

Merged
merged 21 commits into from Apr 4, 2020

Conversation

zhaowenlan1779
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 commented Feb 1, 2020

In an attempt to close #5057, this does the following:

  1. Moves video dumping to its own graphics thread and context. This eliminated Nvidia's warnings that 'pixel transfer is synchronized with 3D rendering'.
  2. I tweaked the projection matrix to flip the image directly, instead of flipping it on CPU while copying. (though I am not sure if I actually understood how the matrix works)
  3. Allows the user to select the formats and encoders they want to use, as well as providing the options to pass to them. For citra-qt, I wrote a good but probably too big frontend (codewise) for easier configuration. Video and audio bitrates are now configurable as well.
  4. Fixes some issues in the FFmpeg backend while setting pixel formats, sample format and sample rate.
  5. Adds a simple error message shown when video dumping fails to start that just tells the user to refer to the log.

It is recommended to review commit by commit and more comments are in the commit messages.

Screenshots:

Dumping dialog
image

Options dialog
image

Set options dialog
image
image
image

Error message
image

TODO:

  • Proper error reporting
  • Fix builds

This change is Reviewable

@zhaowenlan1779 zhaowenlan1779 force-pushed the video-dumping-update branch 4 times, most recently from 6bd9a26 to ed71921 Compare Feb 1, 2020
@zhaowenlan1779 zhaowenlan1779 marked this pull request as ready for review Feb 1, 2020
@jroweboy
Copy link
Contributor

jroweboy commented Feb 8, 2020

Allows the user to select the formats and encoders they want to use, as well as providing the options to pass to them. For citra-qt, I wrote a good but probably too big frontend (codewise) for easier configuration. Video and audio bitrates are now configurable as well.

On this point, this is really really hard to configure ^^; I've been playing around with it, and since every codec and encoder is displayed, it took quite some time to get the settings right. I would have much preferred a "simple" approach where we preselect a few known good encoders and a few "good enough" option sets, but I'm not going to block the change over this. I really just think this is overkill and it'll make it less usable to the average person.

Copy link
Contributor

@jroweboy jroweboy left a comment

One bit of UI feedback, I'm wondering if at the top of the encoder options dialog, if we can put a text box where users can type in options there, or if they change an option through the UI, then it'll add it to the textbox. (Basically make a View for the current options parampackage Model) This way, users will be able to copy paste "best settings" that they find elsewhere.

We still could use a simpler configuration for non technical users, but that doesn't have to happen with this PR.

src/core/dumping/ffmpeg_backend.h Show resolved Hide resolved
.toString()
.toStdString();

Settings::values.video_bitrate =
Copy link
Contributor

@jroweboy jroweboy Feb 8, 2020

Choose a reason for hiding this comment

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

I don't really have a good idea on how to change this, but having a "BitRate" field for video doesn't really handle variable bit rate encoders very well. Maybe its better to just leave the bit rate in the advanced options config window?

Copy link
Member Author

@zhaowenlan1779 zhaowenlan1779 Feb 9, 2020

Choose a reason for hiding this comment

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

If I understand correctly this is the 'target' or 'average' bitrate. The doc says this is unused for 'constant quantizer encoding' (whatever that means), but for most other encoders this setting should be rather important.

@zhaowenlan1779
Copy link
Member Author

zhaowenlan1779 commented Feb 9, 2020

On this point, this is really really hard to configure ^^; I've been playing around with it, and since every codec and encoder is displayed, it took quite some time to get the settings right. I would have much preferred a "simple" approach where we preselect a few known good encoders and a few "good enough" option sets, but I'm not going to block the change over this. I really just think this is overkill and it'll make it less usable to the average person.

While I agree with that, I also think it is important that we provide full control over what options are passed to the encoders, so that advanced users can do whatever they like.

Actually, I also hope that we can have a few preset option sets but I'm no video encoding expert, and don't really know much about the options myself. If someone is kind enough to provide a few sets I will be glad to add them.

@zhaowenlan1779
Copy link
Member Author

zhaowenlan1779 commented Feb 22, 2020

Updated according to @jroweboy's comments

image

@zhaowenlan1779 zhaowenlan1779 requested a review from jroweboy Feb 23, 2020
@MyLegGuy
Copy link

MyLegGuy commented Feb 25, 2020

i wanted to get high quality recording so i found the crf video encoder option in the list and set it to 17. but citra log says:
[ 469.666992] Render <Warning> core/dumping/ffmpeg_backend.cpp:Init:144: Video encoder options not found: crf:17

also PCM signed 16-bit little-endian (pcm_s16le) causes fail with [ 469.670174] Render <Error> core/dumping/ffmpeg_backend.cpp:Init:241: Specified audio encoder does not support any planar format
my output format was flv and video encoder was nvenc_h264

This uses the mailbox model to move pixel downloading to its own thread, eliminating Nvidia's warnings and (possibly) making use of GPU copy engine.

To achieve this, we created a new mailbox type that is different from the presentation mailbox in that it never discards a rendered frame.

Also, I tweaked the projection matrix thing so that it can just draw the frame upside down instead of having the CPU flip it.
The ParamPackage got modified so that we can use range-based for on it
The default values are VP9/libvorbis just like before. The default configuration is provided for VP9
While YUV420P is widely used, not all encoders accept it (e.g. Intel QSV only accepts NV12). We should use the codec's preferred pixel format instead as we need to rescale the frame anyway.
We previously assumed that the first preferred sample format is planar, but that may not be true for all codecs. Instead we should find a supported sample format that is planar.
Previously, we just used the native sample rate for encoding. However, some encoders like libmp3lame doesn't support it. Therefore, we now use a supported sample rate (preferring the native one if possible).

FFmpeg requires audio data to be sent in a sequence of frames, each containing the same specific number of samples. Previously, we buffered input samples in FFmpegBackend. However, as the source and destination sample rates can now be different, we should buffer resampled data instead. swresample have an internal input buffer, so we now just forward all data to it and 'gradually' receive resampled data, at most one frame_size at a time. When there is not enough resampled data to form a frame, we will record the current offset and request for less data on the next call.

Additionally, this commit also fixes a flaw. When an encoder supports variable frame sizes, its frame size is reported to be 0, which breaks our buffering system. Now we treat variable frame size encoders as having a frame size of 160 (the size of a HLE audio frame).
These two functions allow the frontend to get a list of encoders/formats and their specific options.

Retrieving the options is harder than it sounds due to FFmpeg's strange AVClass and AVOption system. For example, for integer and flags options, 'named constants' can be set. They are of type `AV_OPT_TYPE_CONST` and are categoried according to the `unit` field. An option can recognize all constants of the same `unit`.
This dialog allows changing the value and unsetting one option. There are three possible variants of this dialog:

1. The LineEdit layout. This is used for normal options like string and duration, and just features a textbox for the user to type in whatever they want to set.

2. The ComboBox layout. This is used when there are named constants for an option, or when the option accepts an enum value like sample_format or pixel_format. A description will be displayed for the currently selected named constant. The user can also select 'custom' and type in their own value.

3. The CheckBox-es layout. This is used for flags options. A checkbox will be displayed for each named constant and the user can tick the flags they want to set.
This is a simple list of name-value pairs of options. Users can double-click on an option to set or modify its value.
This is the main dialog of video dumping. This dialog allows the user to set output format, output path, video/audio encoder and video/audio bitrate.

When a format is selected, the list of video and audio encoders are updated. Only encoders of codecs that can be contained in the format is shown.
Note that it is only compiled in for FFmpeg video dumper enabled builds
This is just a simple message that tells the user to refer to the log
So that users can just paste a set of parameters they found elsewhere.
For non-planar formats, only the first data plane is used. Therefore,
they need to be handled differently in certain places.
…ntext

As they actually have every encoder/format as their child classes, this will result in a lot of extra options being added.
For easier usage. Also made the option list sortable.
@zhaowenlan1779
Copy link
Member Author

zhaowenlan1779 commented Feb 28, 2020

@MyLegGuy

i wanted to get high quality recording so i found the crf video encoder option in the list and set it to 17.

That option actually does not exist for nvenc, but previously a bug made the list show every possible encoder option. It is now fixed, and only options for the encoder you selected (and generic options) will be displayed.

also PCM signed 16-bit little-endian (pcm_s16le) causes fail with [ 469.670174] Render core/dumping/ffmpeg_backend.cpp:Init:241: Specified audio encoder does not support any planar format

Now non-planar formats are also handled and this shouldn't be an issue anymore.


However your configuration (flv, nvenc, pcm_s16le) still did not work for me, as flv claims that 'pcm_s16le' cannot be contained in flv, despite the codec info fetched from FFmpeg saying that they were compatible. This is not really fixable and you should switch to another format/codec.

The most important one being adding a mutex to protect the format_context. Apparently it wasn't thread safe (as one'd expect) but I didn't think about that.

Should fix some of the strange issues happening with MP4 muxers, etc.
When dumping was stopped, the game will be paused and then resumed. However when the game was already paused this will result in the game being unexpectedly resumed, which isn't what we want.
@BreadFish64 BreadFish64 merged commit 9c7da35 into citra-emu:master Apr 4, 2020
3 checks passed
@zhaowenlan1779 zhaowenlan1779 deleted the video-dumping-update branch Apr 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants