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

Newer codecs (svt_av1, rav1e, svt_hevc) #445

Merged
merged 7 commits into from Jun 2, 2020
Merged

Newer codecs (svt_av1, rav1e, svt_hevc) #445

merged 7 commits into from Jun 2, 2020

Conversation

eisneinechse
Copy link
Collaborator

Initial changes to include new codecs

@codecov-io
Copy link

codecov-io commented Feb 16, 2020

Codecov Report

Merging #445 into develop will increase coverage by 5.93%.
The diff coverage is 11.11%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #445      +/-   ##
===========================================
+ Coverage    42.37%   48.30%   +5.93%     
===========================================
  Files          129      128       -1     
  Lines        13276     9967    -3309     
===========================================
- Hits          5626     4815     -811     
+ Misses        7650     5152    -2498     
Impacted Files Coverage Δ
src/FFmpegWriter.cpp 62.75% <11.11%> (+1.46%) ⬆️
src/effects/Blur.cpp 63.63% <0.00%> (-8.78%) ⬇️
src/Timeline.cpp 38.70% <0.00%> (-2.69%) ⬇️
src/Frame.cpp 46.54% <0.00%> (-0.11%) ⬇️
include/Clip.h 88.88% <0.00%> (ø)
src/Profiles.cpp 0.00% <0.00%> (ø)
include/Timeline.h 68.75% <0.00%> (ø)
src/ReaderBase.cpp 16.57% <0.00%> (ø)
src/TextReader.cpp 0.00% <0.00%> (ø)
src/ChunkReader.cpp 0.00% <0.00%> (ø)
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe8ea21...d9e6af5. Read the comment docs.

@@ -462,6 +462,18 @@ void FFmpegWriter::SetOption(StreamType stream, std::string name, std::string va
case AV_CODEC_ID_AV1 :
c->bit_rate = 0;
av_opt_set_int(c->priv_data, "crf", std::min(std::stoi(value),63), 0);
if (strstr(info.vcodec.c_str(), "svt_av1") != NULL) {
//av_opt_set_int(c->priv_data, "qp", std::min(std::stoi(value),63), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed.

Suggested change
//av_opt_set_int(c->priv_data, "qp", std::min(std::stoi(value),63), 0);

av_opt_set_int(c->priv_data, "forced-idr",1,0);
}
if (strstr(info.vcodec.c_str(), "rav1e") != NULL) {
//av_opt_set_int(c->priv_data, "qp", std::min(std::stoi(value),255), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

Suggested change
//av_opt_set_int(c->priv_data, "qp", std::min(std::stoi(value),255), 0);

av_opt_set_int(c->priv_data, "speed", 7, 0);
av_opt_set_int(c->priv_data, "tile-rows", 2, 0);
av_opt_set_int(c->priv_data, "tile-columns", 4, 0);
//av_opt_set(c->priv_data, "tile-row", "", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's 'tile-row'? I saw it commented out in a few places.

Comment on lines 540 to 545
if (strstr(info.vcodec.c_str(), "aom") != NULL) {
// Hack to set tiles; libaom doesn have qp only crf
av_opt_set_int(c->priv_data, "crf", std::min(std::stoi(value),63), 0);
av_opt_set_int(c->priv_data, "tile-rows", 1, 0); // log2 of number of rows
av_opt_set_int(c->priv_data, "tile-columns", 2, 0); // log2 of number of columns
}
Copy link
Contributor

Choose a reason for hiding this comment

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

"Hack to set tiles?" So if I'm understanding this right, when using the aom codec:

// Encode with a CRF of 15 and.... (no? default?) tiling
writer.setOption(VIDEO_STREAM, "crf", 15)
// Encode with a CRF of 15 and tile-rows=1, tile-columns=2
writer.setOption(VIDEO_STREAM, "qp", 15)

Is that right? Feels a bit magical for my blood. Is there any reason to do it this way, instead of just setting the tile-* values for aom in the crf option processing, the way it's done for rav1e?

@eisneinechse
Copy link
Collaborator Author

In my defence this was just a copy of my tests that I quickly included. That has to be changed. I only wanted that this finally is visible to other people and people can comment on it and improve it.
When I started many things just didn't work and I hacked around.
Now we have to find good default values and we have to find a way to let the user decide what quality is wanted. Maybe just a slider Good/slow <-> Bad/fast.
I for one was confused that a higher value doesn't always mean better quality it can be faster but worse quality. And with newer codec there is not just a bigger file that is better but there are encoding modes that may produce a better quality at lower bitrate but the encoding takes (much) longer.
Tiles have the advantage that the DECODER can use a thread for each tile, that helps the smart phones a lot, but the size is a bit larger and it may change the quality a bit. rav1e can also parallelize the encoding with tiles.
Then, why have 3 encoders? SVT-AV1 uses AVX2 commands that not every CPU has (I don't know if a patch to make svt work on these CPUs is included), rav1e does not scale as good as svt with threads (and it can or might not be compiled with AVX2 support), and libaom is just the standard, still. If I have a CPU with only 4 threads and only a slow AVX2 comand set rav1e may be faster than svt-av1.
And at the end: I can not understand what all of the options of the encoders can do, and they are not the same. This is only a start and the options must be set in the future in a way that is easy and works for all CPUs (GPUs soon?).

@eisneinechse
Copy link
Collaborator Author

I think the crf AND qp is simply a bug. I have to check. I think one should only use one of them.

@eisneinechse
Copy link
Collaborator Author

tiles are in rows and columns. Unfortunately the name of the option my not be the same for different encoders.
Tiles are parts of the complete one frame but AFAIK the the picture is always divided into 2 to the power of x where x is 0, 1, 2, ... . So, a picture can be 4 columns of times wide and 2 rows of times high. BTW when looking for the names of the options be careful they changed from time to time. Just to make it more interesting.

@eisneinechse
Copy link
Collaborator Author

Sorry about the delay. You might have noticed that I am somehow time restrained and this was another one of the times where I had to be somewhere else.
BUT my intention was also to have other encoder because I have a problem with the export and FPSs. There are also issues from other people that have the same problem. The fps value is not 30 as it should be but the ration of num and denum is very close to 30 but not 30, that is when exporting to 30 fps. So far what I have found is:
It is only in mp4 or mov. Not in webm.
It is not limited to h.264 or whatever. So it's the container.
The number of frames is different from what it should be, it is one off. And the wrong fps is calculated by, this is for 30 fps, 30*#ofFrames/(#ofFrames - 1).
I found lines in the source where mp4 and mov is handles differently but Thay didn't change anthing. @ferdnyc maybe you know of a code in there where the number of frames is changed by one (maybe the endframe) and framerate or timescale is altered.
AND most important: This I can only see with ffmpeg 4 libraries compiled into libopenshot.
(To check the frame rate of the exported clip just import it into openshot and look at the file properties)

@eisneinechse
Copy link
Collaborator Author

This comment is misleading. Sorry.
// Hack to set tiles; libaom doesn have qp only crf
It should be:
// Hack to set tiles
// libaom doesn't have qp only crf.
And calling it a hack is also not quite correct, I simply set the tiles and did not know how to quickly change the GUI so I could set them from the export window, so I just set the values to fixed values.
This should be two comments. The tiles have no connection to qp or crf.
Again I was checking settings that at some point did not work and/or were new.
It all comes down to: How can we set option while at the same time have a simple user interface.
I will change some of it and test it.
I/We could really need help finding the fps bug. It doesn't show up easily unless you want to sent the clip to someone that checks it like youtube. These clips won't be accepted because they are not 30 fps! That's bad!

@SuslikV
Copy link
Contributor

SuslikV commented Feb 17, 2020

The number of frames is different from what it should be, it is one off. And the wrong fps is calculated by, this is for 30 fps, 30*#ofFrames/(#ofFrames - 1).

This reminds me the OpenShot/openshot-qt#3138 which is not an issue but the wrong interpretation of the right data by Windows 10. In mp4/mov the samples are calculated and duration of the last one is can be zero (it's normal).

@eisneinechse
Copy link
Collaborator Author

I don't use Windows.
That wrong fps value is only there when I use openshot that uses ffmpeg 4, the same file exported with the AppImage has the correct fps. And AFAIK at some point some time ago the fps value was right, but I could be wrong there. And even openshot itself shows the wrong fps value when I import the exported file. I tested this with my self compiled ffmpeg (versions 4.0 up to the development branch, and with the pre compiled ffmpeg from the ppa).

@SuslikV
Copy link
Contributor

SuslikV commented Feb 17, 2020

Can you provide the file sample (.mp4) ?

@eisneinechse
Copy link
Collaborator Author

wildfire_ff4.mp4.zip

@SuslikV
Copy link
Contributor

SuslikV commented Feb 17, 2020

The wildfire_ff4.mp4 has 77 samples of 512 delta + 1 sample of 0 delta (the last one, so it comply to the ISO) for video, and 121 samples of 1024 delta for audio:

video
track timescale = 15360
track duration = 39424
thus track's duration is 2.56666(6) sec

audio
track timescale = 48000
track duration = 123904
thus track's duration is 2.58133(3) sec (the longest track = the length of the movie)

Both calculations below is not correct! This is wrong calculations for example only!
Because last sample cannot be shown on the screen - it has zero display length (delta).

Thus you may see the fps as
2.56666(6) sec / (77 + 1) (total video samples) or about 30.38961 fps.
or even
2.58133(3) sec / (77 + 1) (total video samples) or about 30.21694 fps

What number do you see as fps for the file in your system?

To be honest, the MediaInfo shows constant FPS for the file.

@eisneinechse
Copy link
Collaborator Author

eisneinechse commented Feb 17, 2020

30.38961 fps both in VLC and when I import the file in openshot it show 2340 / 77.
When I look at the file properties in Openshot at Frame Settings it shows first Frame 1, last frame 78.
If it would be 2340 / 78 we would have the correct 30 fps.

@SuslikV
Copy link
Contributor

SuslikV commented Feb 17, 2020

I don't know what OpenShot shows, but as you see every programmer tries to simplify the things and thus making mistakes. Feel free to make report to the VLC project.

@eisneinechse
Copy link
Collaborator Author

I think it is the last empty video frame. The track duration is 512 (or one video frame) too short. The fps is calculated by dividing the duration by the frame number but when calculating the duration the last empty frame is not taken into account while it is counted as a frame when calculating the frame count.
When youtube doesn't accept the clip because the fps are not 30 fps as required I would say the clip is the problem, not vlc or youtube.

@SuslikV
Copy link
Contributor

SuslikV commented Feb 17, 2020

I have uploaded this file to the YouTube and I don't know what are you talking about. There is no issues in the mentioned file. I think, Europe servers of YouTube are much closer to me. But if you have other results, try to ask the YouTube service itself, why it don't allow to you to upload this video.

@eisneinechse
Copy link
Collaborator Author

If it works I'm OK with that.
I just now included the svt-hevc encoder support too. Simple, needs probably more fine tuning, and I won't comment on the quality of the exported clip as I haven't tuned the parameters yet but it works and is very fast.

@eisneinechse
Copy link
Collaborator Author

@SuslikV Actually the first station where I would have to complain about showing the wrong fps would be Openshot. After importing that clip and then looking at the file properties (Video Details: Frame Rate:) it shows 2340 / 77.
What bothers me the most is that with ffmpeg 3 I don't have the problem.

@SuslikV
Copy link
Contributor

SuslikV commented Feb 18, 2020

...it shows 2340 / 77

Just a mistake in OpenShot. You want to fix it, yourself?

Edit: ...seems to be not.
My attempt is here (only fps, no duration checks)
#448

@jonoomph jonoomph changed the title Newer codecs WIP: Newer codecs Feb 27, 2020
@jonoomph
Copy link
Member

jonoomph commented Jun 2, 2020

Merging now! Thanks Peter!

@jonoomph jonoomph changed the title WIP: Newer codecs Newer codecs (svt_av1, rav1e, svt_hevc) Jun 2, 2020
@jonoomph jonoomph merged commit 2834e77 into OpenShot:develop Jun 2, 2020
Comment on lines +1997 to +2003

if (AV_GET_CODEC_TYPE(video_st) == AVMEDIA_TYPE_VIDEO && AV_FIND_DECODER_CODEC_ID(video_st) == AV_CODEC_ID_RAWVIDEO) {
#else
ZmqLogger::Instance()->AppendDebugMethod("FFmpegWriter::write_video_packet", "frame->number", frame->number, "oc->oformat->flags & AVFMT_RAWPICTURE", oc->oformat->flags & AVFMT_RAWPICTURE);

if (oc->oformat->flags & AVFMT_RAWPICTURE) {
#endif
Copy link
Contributor

@ferdnyc ferdnyc Jun 7, 2020

Choose a reason for hiding this comment

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

(I know this is merged already, but I had one small question.) I guess it would be: do we really need the non-4.0 path here at all?

AVFMT_RAWPICTURE was finally removed in 4.0, but it was marked deprecated waaaay back in 3.0, in this commit:

From: Luca Barbato <lu_zero@gentoo.org>
Date: Mon, 12 Oct 2015 14:06:07 +0000 (+0200)
Subject: avformat: Do not use AVFMT_RAWPICTURE
X-Git-Tag: n3.1-dev~97^2~347
X-Git-Url: http://git.ffmpeg.org/gitweb/ffmpeg.git/commitdiff_plain/34ed5c2e4d9b7fe5c9b3aae2da5599fabb95c02e

avformat: Do not use AVFMT_RAWPICTURE

There are no formats supporting it anymore and it is deprecated.
Update the documentation accordingly.
---

Clearly, even at that time it was already considered legacy code.

Meanwhile, at least in terms of the codec enum, AV_CODEC_ID_RAWVIDEO has existed in its current form since FFmpeg 1.0 — and it only didn't exist before then, because that's when it was renamed from CODEC_ID_RAWVIDEO to the AV_ prefix. But it's easily as old as dirt.

So, it sounds like that code we have to use with libavformat >= 58 is actually the code we should be using on all versions... or at the very least, all versions since 3.0 (libavformat 57) for sure. (And as far as I'm concerned, we should just stop supporting any version earlier than 3.0 — if not later.)

Is there something to recommend sticking with AVFMT_RAWPICTURE in pre-4.0 code? Does it give us any advantage? Or is it just legacy code we could be rid of?

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.

None yet

5 participants