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

Make plane generic over pixel component type #1002

Merged
merged 1 commit into from Feb 21, 2019

Conversation

Projects
None yet
4 participants
@rom1v
Copy link
Collaborator

commented Feb 15, 2019

In order to support both u8 and u16 for plane components, make the Plane structure generic over the component type. As a consequence, many other structures and functions also become generic.

Some functions are not u8-compatible yet, although they have been make generic over the component type to make the compilation work. They assert that the size of the generic parameter is 16 bits wide.

For this reason, the root context structure is unconditionally created as Context<u16> for now.

@rom1v rom1v force-pushed the rom1v:generic branch 2 times, most recently from cf4d348 to c501fc7 Feb 15, 2019

@coveralls

This comment has been minimized.

Copy link

commented Feb 15, 2019

Coverage Status

Coverage decreased (-0.3%) to 79.812% when pulling 32c19dd on rom1v:generic into baec5d3 on xiph:master.

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

FrameInvariants becoming generic really adds a lot of noise :/ this is because of the reference frame structure correct?

@rom1v

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2019

this is because of the reference frame structure correct?

Yes, just because of its field:

pub rec_buffer: ReferenceFramesSet<T>,
@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

Do you think it would make sense to do some dynamic dispatch over that field? Or does that make it even more complicated?

@rom1v

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2019

Do you think it would make sense to do some dynamic dispatch over that field?

That would mean erasing its concrete type behind a trait:

pub trait RFS {}

impl<T: Pixel> RFS for ReferenceFramesSet<T> {}

pub struct FrameInvariants {
    // …
    pub rec_buffer: Box<dyn RFS>,
    // …
}

But then, from a FrameInvariants, we could not expose the Frames of the rec_buffer (because its concrete type would be lost), so every action on the ReferenceFrameSet<T> would need to be implemented through methods on the RFS trait.

It would be impractical IMO.

In fact, ReferenceFrameSet needs to updated with a FrameState<T>, which is not possible if concrete type of the ReferenceFrameSet is not known (unless we also erase the FrameState parameter…).

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

I see. I guess we could also split out ReferenceFrameSet to be its own parameter. But that doesn't make many things better.

@tdaede tdaede requested a review from lu-zero Feb 16, 2019

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

lu_zero can you check how the api.rs changes interact with crav1e?

@lu-zero

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

They would break everything, but I can fix after it lands.

@lu-zero

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

I need to check what works better compared to dyn dispatch, but we could also at a certain point of the chain use just a proxy like this

enum Something {
   Something16(SomethingInner<u16>),
   Something8(SomethingInner<u8>
}

With From and Deref made to unwrap to the actual type.

@rom1v rom1v force-pushed the rom1v:generic branch 3 times, most recently from e10d054 to 8c386e3 Feb 16, 2019

@tdaede

tdaede approved these changes Feb 19, 2019

Copy link
Collaborator

left a comment

I still wish we could have FI not be per-depth but I think this is currently the best option.

Make plane generic over pixel component type
In order to support both u8 and u16 for plane components, make the Plane
structure generic over the component type. As a consequence, many other
structures and functions also become generic.

Some functions are not u8-compatible yet, although they have been make
generic over the component type to make the compilation work. They
assert that the size of the generic parameter is 16 bits wide.

For this reason, the root context structure is unconditionally created
as Context<u16> for now.

@rom1v rom1v force-pushed the rom1v:generic branch from 8c386e3 to 32c19dd Feb 20, 2019

@lu-zero lu-zero merged commit 8057ee7 into xiph:master Feb 21, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.3%) to 79.812%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.