Yalir / sfeMovie

sfeMovie is a simple C++ library that lets you play movies in SFML based applications. It relies on FFmpeg to read medias and remains consistent with SFML's naming conventions.
http://sfemovie.yalir.org/
GNU Lesser General Public License v2.1
114 stars 37 forks source link

Fit function not working properly #69

Closed hsdk123 closed 9 years ago

hsdk123 commented 10 years ago

I'm realising that

fit( x, y, width, height, false );

Doesn't work properly in that if I give a width and height that doesn't match the width/height of the original video, then I expect the movie gets squashed to the width and height indicated - but this doesn't seem to be happening.

Here's a test video: https://github.com/hsdk123/mis_upload/blob/master/test_suzumiya.mp4

Try fit ( 0, 0, 1024, 576, false);

If I play the video without using the fit function at all (just at it's original width/height), the movie is displayed fine in it's original width/height without problems.

feliwir commented 10 years ago

The ration of the movie will never change if you use the fit function, depending on what size you choose to fit , the size of the black borders will change though

Ceylo commented 10 years ago

@feliwir Not with last parameter set to false

Didn't have time to reproduce the issue with the given video but with local media I didn't notice anything suspicious for now.

Ceylo commented 10 years ago

Your video has black borders at the top and bottom hardcoded in the video frames. Is it what looks odd? Apart from that I see nothing wrong.

hsdk123 commented 10 years ago

I notice that the video scales larger than it should. For example, the video should be 1024 x 576, but it looks like it's about 1500 x 700.

I've tried checking the getScale() function every update, and it seems to be coming out with the correct scale value - I don't quite understand why this is happening.

Ceylo commented 10 years ago

Can you apply the following patch and check whether the borders look fine?

diff --git a/src/MovieImpl.cpp b/src/MovieImpl.cpp
index 0d0777e..9c1c222 100644
--- a/src/MovieImpl.cpp
+++ b/src/MovieImpl.cpp
@@ -30,7 +30,7 @@
 #include <cmath>
 #include <iostream>

-#define LAYOUT_DEBUGGER_ENABLED 0
+#define LAYOUT_DEBUGGER_ENABLED 1

 namespace sfe
 {
@@ -235,6 +235,8 @@ namespace sfe
         {
             sfeLogWarning("Movie::update() - No media loaded, nothing to update");
         }
+        
+        m_debugger.bind(&m_videoSprite);
     }

     void MovieImpl::setVolume(float volume)
@@ -527,11 +529,11 @@ namespace sfe
                                            m_videoSprite.getScale().y - 10);
             }

-            m_debugger.bind(&subtitleSprite);
+//            m_debugger.bind(&subtitleSprite);
         }

-        if (m_subtitleSprites.size() == 0)
-            m_debugger.bind(NULL);
+//        if (m_subtitleSprites.size() == 0)
+//            m_debugger.bind(NULL);
     }

     void MovieImpl::didWipeOutSubtitles(const SubtitleStream& sender)
hsdk123 commented 10 years ago

Thanks for the reply, I don't quite understand the patch though - do I erase the lines with - and add the line with +?

I'm currently also using the static_lib branch - the code might be a bit different.

Ceylo commented 10 years ago

Yup that's it :)

bluekirby0 commented 10 years ago

If you are using TortoiseGit then save the contents of the patch in your repository's root directory as "patch.diff" and then right-click on it and select

TortoiseGit->Apply Patch

If you are on linux or using cygwin or MSYS then save the patch as described above and then from the repository root do:

git apply --check patch.diff

Knowing this will help immensely with larger patches.

hsdk123 commented 10 years ago

Thanks for the replies - the static_lib branch's MovieImpl.cpp doesn't seem to have LAYOUT_DEBUGGER_ENABLED , m_debugger, etc. defined though. I don't think I can apply the patch to it.

bluekirby0 commented 10 years ago

You can update your local copy of the branch with the upstream changes from the main repository, but the exact steps to do it are a little complicated. Perhaps Ceylo can carry forward the mainline changes to that branch for you so you can cleanly apply the patch...otherwise you will need to apply it manually...and I don't know offhand how important that LAYOUT_DEBUGGER_ENABLED macro is.

Ceylo commented 10 years ago

@hsdk123 You need to have your local copy of the branch up to date. I don't know if you use master or 48_StaticLib but the patch can be applied on both. You just need to git pull (discard the changes from the patch first)

@bluekirby0 the macro allows easily making the debug box visible or not :) You can live without it of course but here the point is that the local copy is outdated

Ceylo commented 10 years ago

@hsdk123 Note that master (merged into 48_StaticLib too) includes fixes about the fit() method. They don't seem related to your issue though (it's about the video image going out of the window if the top,left point isn't 0,0)

hsdk123 commented 10 years ago

I've just tried copying the code into a file called 'patch.diff', but I'm getting an error:

git apply --check patch.diff
fatal: corrupt patch at line 8
hsdk123 commented 10 years ago

I also get a warning from cmake with the latest version of 48_StaticLib:

CMake Warning (dev) at CMakeLists.txt:120 (add_dependencies):
  Policy CMP0046 is not set: Error on non-existent dependency in
  add_dependencies.  Run "cmake --help-policy CMP0046" for policy details.
  Use the cmake_policy command to set the policy and suppress this warning.

  The dependency target "FFmpeg" of target "sfeMovie" does not exist.
This warning is for project developers.  Use -Wno-dev to suppress it.

Should I just ignore this?

hsdk123 commented 10 years ago

Here's a screenshot of what's currently displaying by the way. The video's only displaying about 3/4 of what it should (the window is 1024/576, the video fit is also set to 1024/576). You can see that only the top black bar's showing when there's also one at the bottom (+ some more stuff on the right that should be showing):

image

hsdk123 commented 10 years ago

I somewhat get the feeling that this might be something related to monitor DPI scaling - I realised that even without fit(), my video was being played in an enlarged size.

I applied the patch from here https://github.com/LaurentGomila/SFML/commit/34933520aca5b20814b5c2734593cee971e949b6 and that now seems to play the movie in the correct size without fit() - fit() still seem to cause this enlargement of the movie though.

I've tried getting the builds to a more recent version of SFML, just to see if it has any further fixes, but it seems that the more recent version of SFML hangs something in movie.play() .

Ceylo commented 10 years ago

Line 8 of the patch is the empty line… odd that it says it's corrupt >< You can probably try to apply the changes manually.

You should not worry about the CMake warning. I know it's here and why, I just didn't look for any solution as it doesn't break anything :) (basically it's because I define dependencies on the FFmpeg target but you have FFmpeg already built so the FFmpeg target isn't created because it's useless and could trigger uncessary rebuild).

As for DPI scaling, I tried it with my screen (rendering 960x540 on a 1920x1080 with scaling) and… indeed the movie size is wrong. However I get the opposite effect as what you show :D The movie image is much too small compared to what is expected.

Does SFML have any advices about how to handle DPI scaling?

bluekirby0 commented 10 years ago

You can also implement some of the resize filters from libswscale (which will probably have much nicer results than whatever SFML uses when applied to compressed video). That will mean requiring at least one of the resize filters being built with ffmpeg but might sidestep the SFML issues (or might not) and improve the quality of the result (unless the only filter available in libswscale is nearest neighbor).