[wip] Vp8 chroma decoding #952
Conversation
|
This may be an issue with color quantization or color space. It could be that the final conversion you are performing is |
|
|
||
| let r = 298.082 * y / 256. + 408.583 * u / 256. - 222.291; | ||
| let g = 298.082 * y / 256. - 100.291* v / 256. - 208.120 * u / 256. + 135.576; | ||
| let b = 298.082 * y / 256. + 516.412 * v / 256. - 276.836; |
There was a problem hiding this comment.
Where do these come from? It would be helpful to get a reference to something here. From googling the floating point numbers, I'd guess: https://www.microsemi.com/document-portal/doc_view/135317-ug0639-color-space-conversion-user-guide
There was a problem hiding this comment.
Seems to be from the ITU-R BT.601 standard
There was a problem hiding this comment.
Definitely not directly: https://www.itu.int/dms_pubrec/itu-r/rec/bt/R-REC-BT.601-7-201103-I!!PDF-E.pdf
There was a problem hiding this comment.
Furthermore, 'Wikipedia' leaves out the precise definition of 'corresponding RGB space' which if you carefully compare the xy-values of blue and green, is not the same as the RGB space defined for sRGB/rec.709.
There was a problem hiding this comment.
In any case, I would have expected the numbers from the section 'without any roundings' to appear here, as we don't have any problems with better precision when we use f32 anyways.
There was a problem hiding this comment.
The numbers come from the linked Wikipedia page, however I will have a look at the standard and move this to a dedicated module with references to the standard.
|
@HeroicKatora it wouldn't surprise me if there are problems with the color conversion, I will have another look at that part as well. However I don't think that is what that is causing the errors that I am thinking about comes from that. Given that the loop filters purpose is to smooth the edges between subblocks and macroblocks I would guess that it should fix some of this "blocky output". I am going to add loop-filtering in this PR, but exam period is closing in so this will maybe advance a bit slowly. |
|
Alright. While I do think that the color conversion is not 100% accurate, it does indeed not explain the blocky artifacts. If you want to continue to search then this is fine. But don't feel pressured, there is no deadline here :) (Reminder: I have one PR open for a few months now as well, that's what drafts are for). |
|
Any news on this? I'd love to be able to use webp, but color support is a blocker. |
|
@est31, I would really like to finish this and I am planning on picking it back up sometime soon, however I can't say exactly when. If somebody else wants and have the ability to continue with this I would be happy if you do. |
|
Hi ! Any news ? What exactly is missing before this can be merged ? |
|
@lovasoa So there are two things. First this patch is very much out of date, so quite some work would be required in order to merge it. Second and the reason this was never finished to begin with is that loop filtering is missing, this causes the output to be quite "blocky". Loop filtering is not entirely trivial to implement, but shouldn't be to bad when following the RFC. |
|
Would it be easier to develop this in its own package? I suppose it would also enable a more exhaustive and tailored interface and improvements in smaller increments with less release pressure. As long as the general structure of the implementation doesn't diverge too much it should be relatively painless to switch to a dependency as compared to in-tree. |
|
@HeroicKatora I definitely think that the vp8 decoder should be extracted into its own crate at some point. However I don't think it is required in order to implement chroma decoding. What would help a lot though, would be to split the vp8 module into multiple files (first commit in this PR). The |
|
@birktj It might be helpful if the PR could be rebased and merged, without the loop decoding implemented, so that it doesn't bitrot any more. The functionality itself could still be disabled, but the only missing item would be loop decoding, which can then be done by someone separately. |
|
I think this PR got superseded by a list of PRs from @tristanphease , eg #1536 #1549 and #1685 , and can thus be closed. |
This is a sort of working implementation of vp8 chroma decoding. It is quite ugly so it requires a little clean-up before it is ready, but it would be nice if people could try it out as I am sure it has a few bugs. The results still don't look entirely correct, but I am wondering if it is just the missing loop-filtering, can anyone confirm/deny this?
I started by splitting the vp8 decoder into some smaller modules to keep my own sanity, so sorry for the large diff.
I am also quite curious as to how the decoder ended up in this state, the actual work wasn't too bad after you got around the code and the reference so it would be interesting to know why it was never finished.
This should fix #939