[RFC] DNxHD support #1857
[RFC] DNxHD support #1857
Conversation
hopefully this should work with a clean history
dnxhd profile requires specific conditions to be met that need more attention put into it. and we can use dnxhr for most cases anyways.
| @@ -721,6 +721,7 @@ bool FFmpegEncoder::SetupCodecContext(AVStream* stream, AVCodecContext* codec_ct | |||
|
|
|||
| if (codec->type == AVMEDIA_TYPE_VIDEO) { | |||
| stream->avg_frame_rate = codec_ctx->framerate; | |||
| stream->time_base = av_add_q(codec_ctx->time_base, {0, 1}); | |||
There was a problem hiding this comment.
What does this do? Is this to ensure a non-zero denominator?
There was a problem hiding this comment.
oh no. That wasn't supposed to make it in, Ill change this back to draft.
but explaining what it is, and what we need it for, we tell the encoder what framerate to use but when writing to an mxf, it also needs to be explicitly told what time_base it's being told to use. this is so that it can ensure interoperability between applications. (one of the reasons people love mxf so much)
just setting stream->time_base = codec_ctx->time_base will make any non-supported framerates error out. which isn't a massive issue since nearly all the video framerates that are exposed by default work fine. with the exception of 10fps and 15fps. so this was never supposed to make it in.
what WAS supposed to make it in, was a better error message explaining why the encode failed immediately instead of error that pops up now "failed to open file: failed to write format header: Invalid argument -22"
the {0, 1} was debugging crap that was supposed to be removed. but I pushed and old commit by accident. but I suppose this is a good time to ask, would it be better to just report why it failed, or find a way to just show the supported framerates?
I had done a fresh install since the time I worked on this so I lost the code either way. and while I don't think anybody who will export DNxHD will try to export 15 or 10 fps I'm not sure if the error message it currently serves is good enough or not.
EDIT: to be clear, the error gets reported to the terminal perfectly fine as Unsupported frame rate 15/1. Set -strict option to 'unofficial' or lower in order to allow it!
There was a problem hiding this comment.
I think an error message should be sufficient, you can see a similar use case here to inform users that H.264 and H.265 require dimensions to be a multiple of 2: https://github.com/olive-editor/olive/blob/master/app/dialog/export/export.cpp#L357
I think using a similar approach for this limitation would be perfectly fine
|
@Quackdoc What's the current status of this PR? I see you're periodically merging master in which to me implies it's ready and you're just maintaining it, but it's still marked as draft. |
|
Not ready yet, there are still some changes I want to make and testing I need to do, however for the time being all my devices are stuck on ffmpeg-git which olive doesn't compile against and I haven't had the time to setup any containers of anything for developing on as they kee[ just "not working" on my computers right now (probably a really bad arch install is to blame) that I cannot remedy. so until the current ffmpeg-git becomes stable and doesn't break everything else or olive starts to compile against and failing either I finally find time to setup proper compilation and development containers, this probably won't progress, however in it's current state it is "usable" for testing. I do plan on merging this... eventually. it's just that right now nothing is lining up. and it's not in a state that is ready for merging. |
|
Olive is about to undergo some largish code refactoring and I wanted to check on the status of old PRs before we do that. Do you plan to update/continue with the PR? Thanks |
|
i can look into updating this after the refactor. |
Sorry for closing yet another PR, but I borked something while trying to clean up commit history. But here is proper full DNxHD support. GUI and .mxf included.
There are a few known issues that I am unsure of how to get around.
the
dnxhdprofile (usually used for 1080p and 720p) needs some special considerations. this is why I have hidden it away for now, we can use dnxhr for most cases that dnxhd would be used anyways. ideally this would be temporary. it would be best to implement it, I am just unsure how to at this moment, let alone the best way to do so. (I believe the issue is setting bitrate, but I have not tested it)the profiles are restricted to pixel format. trying to encode dnxhr_hqx with the default pixel format will not work as it requires 10bit support. I have set the default to standard quality, as it will work with the default pixel format. I have added a tooltip to explain this behavior, but ideally I would like to do this automatically, or at the very least pop up a window notifying the user of why they are getting the error.
I thought about "humanizing" it like we did with h264. however I think it would be detrimental. only really two kinds of people will use dnxhd, those who somewhat know what they are doing, or those who were told to do so. in which case they would also likely be told which profile to use.