-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add yuv4mpeg (Y4M) output #619
Conversation
Getting an Header is:
I compared it to vspipe's y4m header, which does work:
After removing XCOLORRANGE=LIMITED from the header, both ffmpeg and mpv are happy.
|
It might be this change, which increased the max header size in yuv4mpegdec from 80 bytes to 96 in ffmpeg 4.3 - the header quoted above is > 80 bytes long... |
Yep. The current patch works as-is with ffmpeg master. ffmpeg 4.2.4 shipped in Ubuntu 20.04 doesn't like the long header. Removing XCOLORRANGE does not affect ffmpeg master, so we can simply remove XCOLORRANGE to shorten the Y4M header. Note that removing XCOLORRANGE causes 4.2.4 to ignore the field order too, so you get progressive output on top of no explicit range signaling in the container. Not the end of the world and it maintains the status quo with 4.2.4 ,but users may want to upgrade ffmpeg for things to work a little more intelligently. Also the Ubuntu build of 4.2.4 doesn't seem to support monochrome (X264_CSP_I400) in its libx264 wrapper. It's present in libx264.c behind a #define, so maybe there's some magic to enabling it. You would think setting -pix_fmt gray would be enough (it is with static builds from here). |
Ah-hah. Perhaps it would be better to leave it in, and remove the aspect ratio from the header instead. That also seems make it short enough to work with the stock ffmpeg, and is easy to specify manually. |
The libx264 in Ubuntu 20.04 is too old to support monochrome, so that explains that. The "X" options are technically comments, so I'm inclined to drop something like XCOLORSPACE over a "core" field like aspect ratio. However, I understand adding Just throw a new ffmpeg binary in |
return config.outputY4m; | ||
} | ||
|
||
QString MonoDecoder::getHeaders() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can getHeaders be in the Decoder base class, rather than being replicated three times? The code looks the same in all three versions...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started out doing that, but the base class doesn't have a Configuration struct. I gave it a private Decoder::Configuration config, but my C++ skillz are lacking and Decoder::getHeaders() returns nonsense, despite me setting config.xxxx in the derived classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes - the base class can't have a Configuration member because it needs to be one of the subclasses. Leave it as is for now then and I'll have a think...
Well, my thought being is there are non-4:3 aspect LDs as well, and it's not a vital parameter for y4m. Whichever you think is better for compatibility though. |
Good point about anamorphic content. Setting aspect ratio to unknown seems reasonable. Alternatively, could add an anamorphic flag to ld-chroma-decode? Oh, and I didn't try to reduce the fraction. That would save some header space, but maybe not enough to matter? |
We could have a field in the .tbc.json for aspect ratio, and assume 4:3 otherwise - the VBI decoder could pick it up from WSS automatically. (It's also another thing that padding makes tricky, because the output isn't actually 4:3 until you crop off the padding...) |
Yeah. I tried It's a shame they didn't choose a shorter parameter name than colorrange given the header size constraints. But since we're supporting Ubuntu 20.04 LTS, it's probably best to keep it under 80 for the time being. So gotta cut one or the other. Though, what does XYSCSS do? Why is it needed in addition to the C444p16? |
Separate from this patch, I'd argue for mod2 instead of mod8 padding, in part because it borks the aspect ratio. But that's a separate discussion for later. |
It's a limitation of ffmpeg's parser that has been fixed. There's nothing in the format that limits the header size. It's an implementation detail in 2.4.3. Also 'A4:3' isn't correct. That field is for the PAR, not DAR. XYSCSS is because the original mjpegtools implementation of Y4M only had a few formats supported. Everything else is a semi-standard extension. https://git.ffmpeg.org/gitweb/ffmpeg.git/blob_plain/HEAD:/libavformat/yuv4mpegenc.c |
Well, even if not inherent in the format, ffmpeg's implementation is still limited to 96 bytes in master. Otherwise it might have been nice to also include the XLENGTH parameter like vspipe does. |
Updated pull request. Added a field to the .json for signaling widescreen, which is always false for now. The Y4M pixel aspect ratios are calculated before mod8 padding. This makes the DAR correct no matter what padding there may be, even if it isn't exactly 4:3. Also, @Gamnn was correct to ask about the XYSCSS field. Dropping it gives us enough room in the header for even the packaged ffmpeg to "work". YMMV. Respect for auto-setting full/limited range and field order seems to be output codec dependent. Interlacing is set automatically in ffv1, but not libx264. XLENGTH has been added. |
9b00015
to
d7e8ac5
Compare
I spoke too soon. With ffmpeg 4.2.4, adding XLENGTH works for PAL content if it is under 10,000 frames but NTSC is broken no matter what. @Gamnn What actually uses XLENGTH? vspipe writes it, but if x264 and ffmpeg don't use it, what does? |
I was worried CLV discs might push it over the limit.
🤷 Nothing I currently use currently does, so it was just a "might be nice." I'd care more about it if ffmpeg did use it- always nice to see an ETA. |
If there are some variations of options that don't work with older ffmpeg, one option is to have an extra option that adds the additional settings that are convenient but won't work with all version. |
We can fit everything that matters. XYSCSS is for legacy compatibility before the additions to XCOLORSPACE were "standardized." XLENGTH is written by vspipe, but doesn't seem to be read by anything. If we drop both, we can easily fit within the stock ffmpeg's 80-byte header limit. Using ffmpeg master gets you more automatic settings for field order and limited/full range, but even master is not consistent with different codecs (ffv1 will automatically encode interlaced, libx264 will not). Oh, well. Dimensions, frame rate, and colorspace are the big ones and those work fine now. |
I noticed that while -f mono does set it to Cmono16, using --blackandwhite with another decoder doesn't. Other than that, it's looking good. |
That's expected behavior, the same as
Couldn't |
|
Patchset updated.
|
afab756
to
20a0773
Compare
We assume 4:3 for now, but later the VBI decoder could set it automatically.
ld-chroma-decoder: add 'y4m' to the available --output-format options. 'y4m' output is identical to 'yuv' except with the addition of headers. This avoids having to specify the dimensions, frame rate, aspect ratio, and pixel format to downstream consumers such as ffmpeg. For example: ld-chroma-decoder -p y4m input - | ffmpeg -f yuv4mpegpipe -i - output.mkv
Previously, only the mono decoder used GRAY16.
Rebased. Anything left I need to address? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
ld-chroma-decoder: add 'y4m' to the available --output-format options.
'y4m' output is identical to 'yuv' except with the addition of headers. This
avoids having to specify the dimensions, frame rate, field order, aspect ratio,
and pixel format to downstream consumers such as ffmpeg.
For example:
ld-chroma-decoder -p y4m input - | ffmpeg -f yuv4mpegpipe -i - output.mkv