Add test for edid parsing and fix reading gamma
ClosedPublic

Authored by gladhorn on Jul 27 2018, 6:04 AM.

Details

Summary

The bug in reading the gamma value was there from the start (2012),
dividing the number by 100 will result in a truncated value.
Add test so that we can verify that the parsing of the EDIDs work,
it will be easy to add other problematic EDIDs later on.

Let edidDecodeFraction return float

All uses of the function are QQuaternion setters which take floats.
This saves a bunch of conversions and should thus be faster. The edid
test shows that the result is unchanged in all digits.

Diff Detail

Repository
R110 KScreen Library
Branch
arcpatch-D14368
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1261
Build 1275: arc lint + arc unit
gladhorn created this revision.Jul 27 2018, 6:04 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 27 2018, 6:04 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gladhorn requested review of this revision.Jul 27 2018, 6:04 AM
gladhorn updated this revision to Diff 38549.Jul 27 2018, 6:05 AM

Simplify gamma parsing code

gladhorn updated this revision to Diff 38550.Jul 27 2018, 6:07 AM

Minor cleanup

gladhorn updated this revision to Diff 38553.Jul 27 2018, 6:11 AM

Remove extra newline

gladhorn updated this revision to Diff 38564.Jul 27 2018, 8:10 AM

Removed some extra parenthesis

gladhorn updated this revision to Diff 38565.Jul 27 2018, 8:13 AM

Added one more edid

I'm not sure if you could really say that the current code is with a bug, since data[GCM_EDID_OFFSET_GAMMA] should be dividable by 100. I.e. the result is again an integer and nothing gets truncated. But we can still go with the updated code, which looks nicer. Maybe one could add a comment why divide by 100 and add 1.

In regards to the parser test in the edid test:

  • How did you choose the four brands/models to test?
  • Pls order the rows alphabetically by brand/model name.
romangg requested changes to this revision.Jul 27 2018, 9:35 AM
This revision now requires changes to proceed.Jul 27 2018, 9:35 AM

I'm not sure if you could really say that the current code is with a bug, since data[GCM_EDID_OFFSET_GAMMA] should be dividable by 100. I.e. the result is again an integer and nothing gets truncated. But we can still go with the updated code, which looks nicer. Maybe one could add a comment why divide by 100 and add 1.

In regards to the parser test in the edid test:

  • How did you choose the four brands/models to test?
  • Pls order the rows alphabetically by brand/model name.

The old code's output was "2.0" instead of "2.2": integer division first, then cast.

I randomly took the four edid's that were somewhat different from each other that I had available (I had a few more, but they were pretty much the same, so I chose different manufacturers).

I don't see what sorting alphabetically gains us.

gladhorn updated this revision to Diff 38588.Jul 27 2018, 12:34 PM

Removed xfail, it was fixed by the string parsing fix

gladhorn updated this revision to Diff 38590.Jul 27 2018, 12:41 PM

Sort edids in test rows. I still think this is completely pointless and a waste of time.

gladhorn updated this revision to Diff 38592.Jul 27 2018, 12:45 PM
gladhorn edited the summary of this revision. (Show Details)
gladhorn added a subscriber: romangg.

Updated commit message and rebased.

gladhorn updated this revision to Diff 38594.Jul 27 2018, 12:48 PM

arc messup, it squashed another patch into this one

romangg accepted this revision.Jul 27 2018, 1:22 PM

The old code's output was "2.0" instead of "2.2": integer division first, then cast.

Yes, I know. But my train of thoughts was that the original value was multiplied by 100 when the raw edid data was produced, so it should be divisible by 100 again. But the original value was in general already a float, so this is indeed a bug.

This revision is now accepted and ready to land.Jul 27 2018, 1:22 PM
This revision was automatically updated to reflect the committed changes.