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

FFmpeg: Implement image thumbnails #8583

Merged
merged 8 commits into from Dec 24, 2015

Conversation

fritsch
Copy link
Member

@fritsch fritsch commented Dec 13, 2015

This brings the visionary PR of @ace20022 another step further. With this in place we can basically kick all other image dependencies we have and just use the ffmpeg one.

Happy for a thourough review.

return new CFFmpegImage();

return new CXImage(strMimeType);
return new CFFmpegImage();

This comment was marked as spam.

@fritsch fritsch force-pushed the ffmpeg-image-thumbnails branch 5 times, most recently from 69ab3d0 to f8a0dcc Compare December 13, 2015 17:32
@MrMC
Copy link

MrMC commented Dec 13, 2015

very nice, die cxImage die.

@fritsch
Copy link
Member Author

fritsch commented Dec 13, 2015

We are not done yet - sadly this mjpeg encoder only eats YUVJ420P which is deprecated ... so I am foucking arround with sws_scale to alter the range of YUV420P

@fritsch fritsch force-pushed the ffmpeg-image-thumbnails branch 3 times, most recently from cd153bc to 18da43d Compare December 13, 2015 20:34
@MrMC
Copy link

MrMC commented Dec 13, 2015

any speed differences + or - ?

@fritsch
Copy link
Member Author

fritsch commented Dec 13, 2015

Just implemented for now - I did not make any tests yet concerning speed.

@fritsch
Copy link
Member Author

fritsch commented Dec 14, 2015

Original (463 KB):
sws_original

SWS_Rescaled (correct color set - last patch, 281 KB):
sws_right

SWS_Rescaled (wrong color set - without the last patch, 253 KB):
sws_wrong_range

@ace20022
Copy link
Member

I skimmed through it, nice work! 👍

@fritsch
Copy link
Member Author

fritsch commented Dec 14, 2015

@ace20022 yeah - thx for starting it :-)

@stefansaraev
Copy link
Contributor

I am ready to clean up after this is in. the question is: do we want to keep CJpegIO. as it has nothing to do with cximage, and we can not drop libjpeg dependency anyway.

@fritsch
Copy link
Member Author

fritsch commented Dec 14, 2015

@stefansaraev we still need to do some benchmarking - quality wise, file size wise and speed wise. I think I'd like to keep CXImage in there for the first time (but deactivate it) Edit: or not - it can always come back :-)

@stefansaraev
Copy link
Contributor

my question is for cjpegio. not cximage ;)

@ace20022
Copy link
Member

Agree with @fritsch, jpegs are handled different iirc, because cjpegio is ways faster, at least there's a comment somewhere. So we need extensive tests on all platforms.

@fritsch
Copy link
Member Author

fritsch commented Dec 14, 2015

I am pretty new into this "Image stuff" just came into this while emailing with @ace20022 two days ago - others more into the code need to tell us.

@stefansaraev
Copy link
Contributor

sure @ace20022 I am for keeping cjpegio and evenualy removing cximage. but it's up to you guys.

@MrMC
Copy link

MrMC commented Dec 14, 2015

since master is undergoing major construction, nuke out cximage, keep jpgio/gifio and use ffmpeg for everything else. if you keep cximage and fallback to it, you will never find/fix issues.

@fritsch
Copy link
Member Author

fritsch commented Dec 14, 2015

@davilla: can you provide some numbers with: http://sprunge.us/WEKV on top applied?

Here are some results from my testing rigs (all debug build): http://sprunge.us/QEWc

Computing Time: 30.426772 ms Width: 1920 Height: 1080
Computing Time: 0.924089 ms Width: 176 Height: 168
Computing Time: 10.168077 ms Width: 720 Height: 720

@fritsch
Copy link
Member Author

fritsch commented Dec 14, 2015

JpegIO: http://sprunge.us/VWDT

Computing Time: 28.715415 ms Width: 1920 Height: 1080
Computing Time: 0.454397 ms Width: 168 Height: 168
Computing Time: 7.050468 ms Width: 720 Height: 720

Not that bad at all :-)

@fritsch
Copy link
Member Author

fritsch commented Dec 14, 2015

JpegIO Image (352 KB):
jpegio_bench

It's 1.5 times bigger than ours.

@fritsch
Copy link
Member Author

fritsch commented Dec 22, 2015

Then all fine ... I only read the addon code for libraw, sorry for that. So yes - then we have a solution for the kodi clue part. Sorry!

@fritsch
Copy link
Member Author

fritsch commented Dec 22, 2015

FFmpeg's Decode method has a scaling method: fritsch@c9ed88e so whenever the given texture targets are too small images are rescaled, so if it's going through that, they are rescaled. Though there is a little overhead (one additional full copy) that could be solved by static ImageScaler, that btw. FFmpegImage could then reuse again.

@ace20022
Copy link
Member

ImageScaler

That's what I had in mind.This could also be an addon, probably not, or?

@fritsch
Copy link
Member Author

fritsch commented Dec 22, 2015

Yes. We also have encoders that user internal ffmpeg contexts. So an addon depending on sws_scale is doable. Lifetime of buffers, especially resulting buffers need to be discussed but I think @notspiff already has thought through a possible poc that would help us in that regard.

@wsnipex
Copy link
Member

wsnipex commented Dec 22, 2015

looks fine depends wise. Not sure if ffmpeg needs external libs for mjpeg or png, but we have at least libpng available. If it needs libpng, then you should add it to ffmpeg depends in https://github.com/xbmc/xbmc/blob/master/tools/depends/target/Makefile#L103

@fritsch
Copy link
Member Author

fritsch commented Dec 22, 2015

@wsnipex thx for looking, I think it is not needed: https://ffmpeg.org/general.html#Image-Formats it does it internally.

@stefansaraev stefansaraev added Type: Feature non-breaking change which adds functionality v17 Krypton labels Dec 22, 2015
@stefansaraev stefansaraev added this to the K***** 17.0-alpha1 milestone Dec 22, 2015
@fritsch
Copy link
Member Author

fritsch commented Dec 23, 2015

jenkins build this please

MartijnKaijser added a commit that referenced this pull request Dec 24, 2015
@MartijnKaijser MartijnKaijser merged commit 2bdfef6 into xbmc:master Dec 24, 2015
@@ -153,6 +153,8 @@ CFLAGS="$CFLAGS" CXXFLAGS="$CXXFLAGS" LDFLAGS="$LDFLAGS" \
--enable-libvorbis \
--enable-muxer=ogg \
--enable-encoder=libvorbis \
--enable-encoder=png \

This comment was marked as spam.

This comment was marked as spam.

@Memphiz
Copy link
Member

Memphiz commented Jan 6, 2016

mhhh jpegio was nuked now and we still link kodi against libjpeg for darwin here:

https://github.com/xbmc/xbmc/blob/master/tools/darwin/Configurations/App.xcconfig.in#L28

What am i missing?

@fritsch
Copy link
Member Author

fritsch commented Jan 6, 2016

@Memphiz this PR did not nuke it, though @stefansaraev do you have an idea?

@stefansaraev
Copy link
Contributor

huh. I missed that as jenkins built all fine. will add to #8772

@notspiff
Copy link
Contributor

notspiff commented Jan 6, 2016

it wouldn't show up as an error. the library is there (needed by texturepacker) and the linker simply doesn't import any symbols since there are no references.

@Memphiz
Copy link
Member

Memphiz commented Jan 6, 2016

Nope texturepacker is native :D
it built fine because we still build libjpeg-turbo for target in depends (for pythonmodule-pil needs it) - we can drop the link in kodi then - thx for taking care of it @stefansaraev - not sure if other platforms link libjpeg into kodi aswell though ...

@notspiff
Copy link
Contributor

notspiff commented Jan 6, 2016

true that. my point is still valid though ;)

other platforms have been pruned (configure does things properly and have no hard coded link list afaik).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet