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

Replace custom image parser with Skia #582

Merged
merged 3 commits into from Jan 20, 2019

Conversation

Projects
4 participants
@nvllsvm
Copy link
Member

commented Jan 13, 2019

Simplifies our code a little bit. Note that this is slower than the old method, but not enough to warrant the old code.

When skia was the first to read, an 48 distinct executions average to:

externallib 0.000075
original 0.000219
skia 0.000300

When original was the first to read, an 48 distinct executions average to:

externallib 0.000014
original 0.000048
skia 0.000046

When externallib was the first to read, an 48 distinct executions average to:

externallib 0.000187
original 0.000200
skia 0.000046
@ploughpuff

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Apart from the simplification, was the original code defective?

As an alternative to SkiaSharp, has System.Drawing been considered, which apparently can give you the dimensions without loading the entire image:

https://docs.microsoft.com/en-us/dotnet/api/system.drawing.image.fromstream?view=netframework-4.7.2#System_Drawing_Image_FromStream_System_IO_Stream_System_Boolean_System_Boolean_

@JustAMan
Copy link
Member

left a comment

Less code to maintain is good!

@JustAMan

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@ploughpuff

Apart from the simplification, was the original code defective?

It was not supporting new image formats (aside from being non-standard stuff). New image formats were added in a separate library by @Bond-009, but that introduces a one more dependency.

@Bond-009

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

And no one understood it.
And it was badly written.

@ploughpuff

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Yes original was cut+paste from StackOverflow.

Regarding my System.Drawing comment, it seems that route was Win32 only until relatively recently. There now exists a Window Compatibility NuGet Package which offers it:
https://www.nuget.org/packages/Microsoft.Windows.Compatibility

...but we're probably best avoiding that beast??

@Bond-009
Copy link
Member

left a comment

As I said on matrix, this isn't the best way to use the ImageEncoder to get image sizes.
You should throw out the whole function with allowSlowMethods

@nvllsvm

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@Bond-009 - I can do that in another pull request as this keeps the status quo. Thoughts?

@nvllsvm

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

@Bond-009 removed allowSlowMethods and rebased to dev.

@nvllsvm nvllsvm merged commit 784eac6 into jellyfin:dev Jan 20, 2019

1 check passed

continuous-integration/drone/pr Build is passing
Details

@joshuaboniface joshuaboniface added this to In progress in 10.1.0 Release via automation Jan 20, 2019

@joshuaboniface joshuaboniface moved this from In progress to Done in 10.1.0 Release Jan 20, 2019

@joshuaboniface joshuaboniface referenced this pull request Jan 21, 2019

Merged

Release 10.1.0 #651

@nvllsvm nvllsvm deleted the nvllsvm:image branch Jan 23, 2019

@Bond-009

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

Just noticed you moved all Skia stuff inside Emby.Drawing, why?

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.