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

Simplify bitstream and pixel format handling #154

Closed
wants to merge 4 commits into from
Closed

Simplify bitstream and pixel format handling #154

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 19, 2015

This series of commits modifies and simplifies the bitstream. Since xdec, ydec, bit depth and number of planes is derived from the pixel format (e.g. "yuv420p" -> 8 bits/pixel, xdec = ydec = {0,1,1}, planes = 3) it's much simpler to simply send out a single unsigned integer describing the pixel format code.
Granted, this limits the total amount of pixel formats ever supported to 254 (255 is used as an error return code), however it's unlikely that any encoder can support 254 completely different formats.
An array was used to look up and store all different parameters of a pixel format, with it's order taken from the example_encoder file which looks up the pixel format of a raw y4m file.
This commit reduces the total amount of data that needs to get sent for the header by 7 bytes. While this is unlikely to have a big impact on filesize, even at low bitrates, this will make the job of parsing the bitstream easier.

Rostislav Pehlivanov added 4 commits October 19, 2015 20:02
Does not modify anything, code will be activated with the next commits.
This code is what's actually encoded.
Uses the previous commits. Does not modify anything.
This commit is what activates the previous commits.
@tmatth
Copy link
Member

tmatth commented Oct 19, 2015

@xiphmont ?

@kodawah
Copy link
Contributor

kodawah commented Oct 19, 2015

i like the idea, just not sure if it should go in a separate file

i would suggest to use the pixel format descriptions names from ffmpeg/libav using av_get_pix_fmt_name()
in this way it would simplify having to store a separate lookup table for the conversion and would trim a couple of duplicates

@ghost
Copy link
Author

ghost commented Oct 19, 2015

I left it in a separate file as there was really no other place for it to go without duplication. I'd normally just have the entire file as a header file and mark all functions as inline since they're small but the coding style here doesn't really allow for that.
I think it's better to maintain an internal list of supported strings since they're universal and rarely deviate from program to program (e.g. "yuv422p10le" always means 10 bit, subsampled horizontal, little endian format) rather than a huge enum of entries which are originally from another project. Additionally API users are more familiar with it rather than manually setting xdec and ydec, but that's just my opinion.

@tdaede
Copy link
Contributor

tdaede commented Oct 19, 2015

So I generally like the bitstream changes - coding stuff like xdec 7 makes no sense, due to CfL/qm/etc we can only sanely support a limited set of chroma formats anyway.

The current list of formats is kind of non-orthogonal though. There isn't really any reason to include bit depth with chroma subsampling - we can easily support all possible combinations. Also, I'd leave the alpha plane on it's own - why not 4:2:0 with an alpha plane?

Also, not really a fan of the strings helper function - it's convenient for encoder_example parsing y4m files, but the string formats aren't really used inside ffmpeg (it uses AVPixelFormat instead), so it's not very useful there.

@kodawah
Copy link
Contributor

kodawah commented Oct 20, 2015

incidentally it'd be nice that od_image received the same treatment

@kodawah
Copy link
Contributor

kodawah commented Oct 26, 2015

I'm proposing an alternative version here #160

@ghost ghost mentioned this pull request Oct 26, 2015
@ghost ghost closed this Nov 17, 2015
This pull request was closed.
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.

3 participants