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

DVDVideoCodecFFmpeg: Do not confuse users with our fallback deinterlacer #10316

Closed
wants to merge 2 commits into from

Conversation

@fritsch
Copy link
Member

commented Aug 20, 2016

When sw decoding we create a filter chain where we add our yadif deinterlacer which does nothing with frames that are not interlaced, but it is written in ProcessInfo and users are confused.

This just write "none" if we are not interlaced. It would be better to not create the filter_graph at all I think.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2016

Alternatively, if we don't want the filtergraph and can live with an "unkown":

From 9c6680549673b0f861797bc646d4977c847cf470 Mon Sep 17 00:00:00 2001
From: fritsch <fritsch@kodi.tv>
Date: Sat, 20 Aug 2016 16:58:21 +0200
Subject: [PATCH 1/2] DVDVideoCodecFFmpeg: Only construct filter graph if we
 are interlaced

---
 xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp b/xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp
index 54b60c0..9d3b3a4 100644
--- a/xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp
+++ b/xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp
@@ -545,7 +545,8 @@ void CDVDVideoCodecFFmpeg::SetFilters()
       }
   }

-  if (filters & FILTER_DEINTERLACE_YADIF)
+  // only construct the yadif graph when we are interlaced
+  if (m_interlaced && (filters & FILTER_DEINTERLACE_YADIF))
   {
     if (filters & FILTER_DEINTERLACE_HALFED)
       m_filters_next = "yadif=0:-1";
-- 
2.7.4
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 20, 2016

this is wrong. the deint method is adaptive regardless of the interlace flag. I won't go into the same discussion as with the old codec screen. if users don't understand the meaning of the parameters, they should not look at it.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2016

It is wrong to write a deint method into processinfo if none is used...

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 20, 2016

read the docs of ffmped. it defines adaptive as a deint method and this is what we use.

@99rook99

This comment has been minimized.

Copy link

commented Aug 20, 2016

Showing a deinterlacing process, when showing progressive content, could cause needless posts about Kodi deinterlacing progressive content. I'm just thinking of all the Kodi is not playing at 4k, because people see the GUI at 1080.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2016

@FernetMenta I read the docs and I know that nothing happens - but in fact I was asked in the forum why yadif would be active ... explaining them, that yadif does nothing cause we use it 1:-1:1 and adding the filter_graph is no problem and so on I want to do exactly once (!) and not 200 times for every screenshot.

The info is correct, cause one can see this 1:-1:1 part of the yadif string ... which only experts know ... for the interested reader: yes it is correct, cause the last 1 in there only deinterlaces if an input field is tagged interlaced.

screenshot000

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2016

But as the PR is technically wrong, I will close it.

@fritsch fritsch closed this Aug 20, 2016
@99rook99

This comment has been minimized.

Copy link

commented Aug 20, 2016

                                                                                  ‎It may be technically wrong, but for users (some who might not be good with technology) this PR is needed. Unless you want a lot of needless posts on the forum.                                                                                                                                                                                                                                                                                                                                         Sent from my BlackBerry 10 smartphone on the Koodo network.                                                                                                                                                                                                                From: Peter FrühbergerSent: Saturday, August 20, 2016 15:54To: xbmc/xbmcReply To: xbmc/xbmcCc: Adrian Kelly; CommentSubject: Re: [xbmc/xbmc] DVDVideoCodecFFmpeg: Do not confuse users with our fallback deinterlacer (#10316)But as the PR is technically wrong, I will close it.

—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/xbmc/xbmc","title":"xbmc/xbmc","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/xbmc/xbmc"}},"updates":{"snippets":[{"icon":"PERSON","message":"@fritsch in #10316: But as the PR is technically wrong, I will close it."}],"action":{"name":"View Pull Request","url":"https://github.com/xbmc/xbmc/pull/10316#issuecomment-241225990"}}}

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

We had endlelss of those useless discussions with the old codec screen. In case this continues with new process info dialog, I will remove it again.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

If the discussion is the problem, then you can remove it directly - come on. If it's all about adding an (adaptive) string behind it - then we should go for the latter.

I am quite sure there are ~4 people in team kodi (including you and me) that know what yadif=1:-1:1 means without looking it up. If that cryptic thing was the intention then I don't know why we now have a special PlayerDebug thing.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

like the codec screen this new dialog was never targeted to the average joe. it is not importand what is exaclty displayed. adaptive, motion adaptive, temporal, etc. if one does not know the process (what those parameters mean to VideoPlayer), this info is useless anyway.
if it makes you happy, change the string to whatever you like. consider that decoder specific names can easily be loked up.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2016

strange thing, I was in impression that playerdebug was for developers, processinfo for average joe.

however, neither of those has the (correctly shown) info that even "above-than-average-joe" needs and can understand. uhm

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

I don't know what makes me happy. From a dev point of view I think it's also useless, cause from that info you cannot say if it currently has an effect at all. It might be in use (sw decoding of interlaced content), it might be a fallback (as to be seen in above screenshot).

If one debugs a performance issue, this does not help at all. Consider user set Deinterlace-Method to "Off" but he decodes interlaced content in software, then this would automatically enable the yadif deinterlacer again, cause this "adaptive" setting overrules user settings and it is automatically added when the filter_graph is created.

I see at least two problems:
a) This info does not help developers as they don't know if it is active at all
b) The filter graph is constructed and used even if None is chosen as deinterlacer whenever sw decoding is active. I don't even know how another method != yadif can now be chosen when we sw decode.

Let's rephrase this: What would make you happy? (Yeah, remove of all that stuff again ;-)) - No, the other way round: what could we do, so that this ProcessInfo makes more sense for everyone?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

Consider user set Deinterlace-Method to "Off" but he decodes interlaced content in software, then this would automatically enable the yadif deinterlacer again

read the code. off is off

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

it always comes down to the same thing. if you don't know what the code does, the info is useless for you

@99rook99

This comment has been minimized.

Copy link

commented Aug 21, 2016

That is silly, someone doesn't need to know what the code does for it to be helpful to you. I now know that if I see yadif 1:1:1 just to ignore it, but I still have no idea what it is doing. As a developer it is right to do it the way that you are doing it, but for the end user it is wrong and will cause needless questions

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

@FernetMenta: https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp#L509 calls GetFallBackMode which returns VS_INTERLACEMETHOD_DEINTERLACE

so:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp#L514 never matches as the wrong ProcessInfo value overwrites it.

=> Bug in the code.

So, concerning to your question: If one thinks to know what the code does, this info is even more useless :-)

Reproduce: Play an interlaced file of any sort with sw decoding and set deinterlace to inactive and try to disable YADIF, you won't be able to disable it.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

The code is actually missing a check for VS_INTERLACEMETHOD_NONE in the first link above and for all other sw methods, too ... if this is not checked -> the code overwrites user selection and all our render deinterlacers won't work anymore.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

@fritsch right, that is a bug. good catch

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

Yeah - I will fix it the day after tomorrow.

@fritsch fritsch reopened this Aug 21, 2016
@fritsch fritsch force-pushed the fritsch:yadifprocessinfo branch from f717f56 to 0d36619 Aug 21, 2016
@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

I thinks these two should do it. But as I did not fully get the logic of the two lines I removed, I am happy for input. Normally the render should be asked for its AutoInterlace Method. That means we also only add the yadif to the graph whenever user selects it manually.

@FernetMenta could you say something about the reasoning for this two line I removed?

@fritsch fritsch force-pushed the fritsch:yadifprocessinfo branch from 0d36619 to 98dbcb8 Aug 21, 2016
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

  1. Autointerlace of renderer is maximum crap and won't survive v18
  2. deint method is stored in video db with file. assume the deint method is vdpau temporal. now you disable vdpau and play the video. you still want deinterlacing but now you need a different method
@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

Yeah - that's right. I think the codec should not cope with that. I also want to avoid a dependency on the renderer itself. What do you suggest?

I think the easiest thing would be to remove this "per video" setting and add an option to set the default deinterlacing method in the menu.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

just add method none to the condition of the if clause

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

That will still break our render deinterlacers when sw decoding, but better than before

@fritsch fritsch force-pushed the fritsch:yadifprocessinfo branch from 98dbcb8 to caac505 Aug 21, 2016
@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

I would propose a method: IsHWDeinterlacer() - if that returns true, we add a fallback - else not.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

heh? SetFilters is only called for sw decoding

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

Yes, exactly. That's the reason. If we are called with e.g. VDPAU_TEMPORAL and we find out that this method is not existent, e.g. !m_pHardware we need to be prepared and have this YADIF as fallback. I thought this was all about it.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 22, 2016

  • make process info hold a list of currently available deint methods. those can be updated (or trigger update) in RenderManager::Configure
  • pass current deint method to getFallbackDeintMethod and return fallback if not in the list
  • gui calls to VideoPlayer::Supports can be directed to processInfo instead of renderer
@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2016

I don't like that approach for 3 reasons:
a) RenderManager needs to know which decoder (e.g. pictureFormat is related to which deinterlacing / scaling / whatever method)
b) a) could be encapsulated into processInfo, though we would get a direct connection between RenderManager and processInfo, while this would then be used / called from various points in the code
c) if we decide for that approach a whole lot of ifdeffery would come into processInfo to map the relation between the deinterlace methods and the hw dec formats. Only the deint methods are available via IPlayer.h without ifdefs

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 22, 2016

no single ifdef is required. Render does already know, it implements all the Supports methods. we just need to loop over all deint methods and push the supported ones to processInfo.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2016

Superseeded by: #10323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.