10000 Fujifilm X-A20 support by kmilos · Pull Request #610 · darktable-org/rawspeed · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fujifilm X-A20 support #610

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fujifilm X-A20 support #610

wants to merge 1 commit into from

Conversation

kmilos
Copy link
Collaborator
@kmilos kmilos commented Jan 19, 2024 •
edited
Loading

Haven't actually used the new "unknown-no-samples" tag (as first planned) because it is basically the same as the supported X-A10.

@kmilos kmilos requested a review from LebedevRI as a code owner January 19, 2024 15:12
@codecov Codecov
Copy link
codecov bot commented Jan 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9e34fb6) 59.19% compared to head (babce5c) 59.19%.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop     #610   +/-   ##
========================================
  Coverage    59.19%   59.19%           
========================================
  Files          251      251           
  Lines        14869    14869           
  Branches      2001     2001           
========================================
  Hits          8801     8801           
  Misses        5952     5952           
  Partials       116      116
8000
Flag Coverage Δ
benchmarks10.50% <ø> (ø)
integration 46.70% <ø> (ø)
linux 56.92% <ø> (ø)
macOS 20.77% <ø> (ø)
rpu_u 46.70% <ø> (ø)
unittests 18.30% <ø> (ø)
windows ∅ <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LebedevRI
Copy link
Member

Thank you for looking into this.

The thing is, i really don't want to add explicit support for something,
when it fails to meet the minimal requirements, namely: a sample on RPU.
On the other hand, i'd gladly take an entry similar to that of 8b50aa4,
which, as discussed, may actually help us get said sample.

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 19, 2024

On the other hand, i'd gladly take an entry similar to that of 8b50aa4,
which, as discussed, may actually help us get said sample.

Well, that's how I started... ;)

@LebedevRI
Copy link
Member

On the other hand, i'd gladly take an entry similar to that of 8b50aa4,
which, as discussed, may actually help us get said sample.

Well, that's how I started... ;)

I'm sorry, i'm failing to comprehend the meaning.
What i'm saying is that i don't want to add new cameras.xml entries with
mode not being one of yes, no, unknown, unknown-no-samples.

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 19, 2024

I'm sorry, i'm failing to comprehend the meaning.

I was going to add this one as "unknown-no-samples", but then I figured it is sort of supported (not entirely unknown).

As I mentioned in the discussion way before, I don't really see any difference between "no-samples" and "unknown-no-samples" in the darktable UI, the warning message is the same in the current implementation. Perhaps it'll come in the future...

I'll change the PR anyway.

@LebedevRI
Copy link
Member

I was going to add this one as "unknown-no-samples", but then I figured it is sort of supported (not entirely unknown).

Right, i'm not arguing with that. I'm sure it works with the entry added.
My point is that i don't want to add said entry (other than <ID>)
with no sample :S (== no test coverage)

As I mentioned in the discussion way before, I don't really see any difference between "no-samples" and "unknown-no-samples" in the darktable UI, the warning message is the same in the current implementation. Perhaps it'll come in the future...

Is that with submodule updated, or just edited cameras.xml?
With unknown-no-samples it will completely refuse to load in darktable.

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 19, 2024

Is that with submodule updated, or just edited cameras.xml?

Good point, I'll retest when I get a chance.

@LebedevRI
Copy link
Member 8000

Is that with submodule updated, or just edited cameras.xml?

Good point, I'll retest when I get a chance.

Note that i suspect you'll need to update black level copying code in imageio_rawspeed.

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 22, 2024 •
edited
Loading

Note that i suspect you'll need to update black level copying code in imageio_rawspeed.

Hm, it fails building as a dt subproject even before getting to imageio_rawspeed. It build fine standalone though?!

C:/msys64/home/kmilos/darktable/src/external/rawspeed/src/librawspeed/common/Cpuid.cpp:37:8: error: '__get_cpuid' was not declared in this scope
   37 |   if (!__get_cpuid(1, &eax, &ebx, &ecx, &edx))
      |        ^~~~~~~~~~~
C:/msys64/home/kmilos/darktable/src/external/rawspeed/src/librawspeed/common/Cpuid.cpp:40:16: error: 'bit_SSE2' was not declared in this scope; did you mean 'WITH_SSE2'?
   40 |   return edx & bit_SSE2;
      |                ^~~~~~~~
      |                WITH_SSE2

I guess I need to sync up some of the dt cmake modules as well?

@LebedevRI
Copy link
Member

Huh, that makes no sense. You have removed the build directory first, right?

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 22, 2024

Huh, that makes no sense. You have removed the build directory first, right?

Yep... It's almost as if the system <cpuid.h> doesn't get included, i.e. common/Cpuid.h gets priority and is included twice?!

@LebedevRI
Copy link
Member

Hm, it fails building as a dt subproject even before getting to imageio_rawspeed.

I just tried locally, and it works fine for me.

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 22, 2024

I just tried locally, and it works fine for me.

Windows? Could be because it can't tel between "Cpuid.h" and <cpuid.h>? But why when it's only as subproject and not standalone?

@LebedevRI
Copy link
Member

I just tried locally, and it works fine for me.

Windows?
No.

Could be because it can't tel between "Cpuid.h" and <cpuid.h>? But why when it's only as subproject and not standalone?

None of that stuff has been touched for a while now.
Does dt master compile for you as-is?

@LebedevRI
Copy link
Member
LebedevRI commented Jan 22, 2024 •
edited
Loading

FWIW, if i rename common/Cpuid.h file to common/cpuid.h (and fix includes),
it does fail just the way you've described. So the checkout must be on a case-sensitive filesystem.
But why does it fail for you now? Sounds like you changed something on your side.

@kmilos
8000 Copy link
Collaborator Author
kmilos commented Jan 22, 2024

Does dt master compile for you as-is?

Yep, no problem if I go back on dt master w/

cd src/external/rawspeed
git checkout 1e505de
cd ../../..
rm -rf build

@LebedevRI
Copy link
Member

Well, can you git bisect it then please?

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 22, 2024

Well, can you git bisect it then please?

Can try when I get another chance.

@kmilos
Copy link
Collaborator Author
kmilos commented Jan 22, 2024 •
edited
Loading

Well, can you git bisect it then please?

Supposedly Windows dt build didn't like c2a68bb

target_include_directories(rawspeed_common SYSTEM PUBLIC "${RAWSPEED_SOURCE_DIR}/src/external") perhaps makes common/Cpuid.h come before <cpuid.h> on case-insensitive platforms?

@LebedevRI
Copy link
Member

Right. That kind of makes sense i suppose.
(I really should find energy to kill last remnant of intrinsics and remove those files.)

LebedevRI added a commit to LebedevRI/rawspeed that referenced this pull request Jan 22, 2024
@LebedevRI
Unbreak build on case-insensitive fs
0c890f1 
For some reason `<cpuid.h>` first searches in current directory,
just like it's `""` form, and after c2a68bb,
`Cpuid.h` is in the current directory of the compiler for `Cpuid.cpp`,
which results in wrong file being included (global `<cpuid.h>` not being included)

darktable-org#610 (comment)
@LebedevRI LebedevRI mentioned this pull request Jan 22, 2024
Merged
@kmilos
Copy link
Collaborator Author
kmilos commented Jan 22, 2024

With your imageio_rawspeed.cc patch, this now indeed fails to load as any other unknown camera, so good to go I guess...

8000 @kmilos kmilos closed this Jan 22, 2024
@kmilos kmilos deleted the fuji_xa20 branch January 22, 2024 17:13
@kmilos kmilos restored the fuji_xa20 branch February 1, 2024 08:55
@kmilos
Copy link
Collaborator Author
kmilos commented Feb 2, 2024

Note to self: reopen PR before making changes to the branch. 🤦

@LebedevRI
Copy link
Member

Yup, github is best, isn't it?

@kmilos
Copy link
Collaborator Author
kmilos commented Feb 2, 2024

Yup, github is best, isn't it?

It's not just a permission issue, I really have to create a new one?

@LebedevRI
Copy link
Member

It's not a permission issue, i regularly hit that too.

@kmilos kmilos mentioned this pull request Feb 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LebedevRI LebedevRI LebedevRI left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kmilos @LebedevRI
0