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

Subsampling begone #160

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Subsampling begone #160

wants to merge 3 commits into from

Conversation

kodawah
Copy link
Contributor

@kodawah kodawah commented Oct 26, 2015

this PR aims at simplifying the API so that it is no more necessary to carry over sub-sampling values for each image

  • a set of pixel format values is introduced so that the library and applications can easily derive them when necessary (with a private table and with simple ifs respectively)
  • applications don't have to initialize subsampling themselves, so it's less error prone
  • header is trimmed so that only pixel format, planes and bitdepth are stored
  • there are some simplifications here and there in the library itself

I'd like to make a few modifications to this pr, but I wanted to put it out here so that there can be a proper discussion about it

There are no practical usecases for image formats with different bitdepth
across planes, and currently the code assigns/reads the same value
for each plane.
@ghost
Copy link

ghost commented Oct 26, 2015

I guess it would work, though it does basically the same as PR #154 but in a lot more complex and convoluted way. It doesn't simplify example_encoder either. I do like how (in theory, though currently not in practice) it would be able to work with every bit depth value without duplication, though in my PR duplication is nothing more than a few lines to alias the pixel format names. I dislike how it has to look up the array to fetch a decimation every single time it's needed. IMO this should be left for the infodec to do, like in my patch, though I might be a bit biased here.
In total, what's wrong with my patch? I got no solid pointer to use to improve it, so I don't know if it was so fundamentally wrong so that this patch needed to be made or just needs a few minor tweaks.

@kodawah
Copy link
Contributor Author

kodawah commented Oct 26, 2015

hey @atomnuker, thanks for your comments

Yes this PR is similar to yours, and it does change a lot more code but I wouldn't say it's convoluted, just starting from a different perspective: I wanted to get completely get rid of subsampling information because it's something that should not be required (both for initialization and image handling) as it's constant across the board, and I didn't want to add a new APIs, since imho it's not in daala scope to expose how pixel format are handled.

So while examples might not be as simplified as in your case (even though they still are) the semantic required to use daala are heavily reduced, the header size is decreased while retaining the ability to describe every pixel format without increasing the lookup table every time, and applications that already have a sane pixel format handling don't have to fiddle with API calls.

@lu-zero
Copy link
Contributor

lu-zero commented Oct 29, 2015

+1

I'm sure Tim does not want to support many, strange, pixel formats since I pestered him at least twice about it.

Trading some flexibility for some stronger warranties that the encoder behaves correctly sounds good to me.

@tmatth tmatth changed the title Subsamping begone Subsampling begone Nov 17, 2015
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.

2 participants