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

fixed: always update stream details from player (fixes #15584) #15589

Merged
merged 1 commit into from Mar 4, 2019

Conversation

Projects
None yet
8 participants
@arnova
Copy link
Member

commented Feb 23, 2019

Description

This fixes a regression introduced by v18. Basically we should always update the stream details from the player, no matter what.

Motivation and Context

It fixes a regression bug ;-)

How Has This Been Tested?

No brainer

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@arnova arnova requested a review from pkerling Feb 23, 2019

@arnova arnova added the v18 Leia label Feb 23, 2019

@arnova arnova added this to the Leia 18.2-rc1 milestone Feb 23, 2019

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

@FernetMenta : I assume you're also fine with this?

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

What about where some of the details are not in the stream itself but provided by say an addon that started the playback? I don't if know that happens, but I know that some item properties can be set via Python so is it possible?

The "get_stream_details_from_player" property may not have been implemented/updated correctly, but there must have been a reason for it, and are you sure that reason is no longer valid?

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

No I'm not sure about the original "reason" but I do know we always want to update stream details, like we always did in v17 (and older). An additional benefit of doing this is for (low powered) systems where auto stream detail extraction is disabled, and we still retrieve stream details this way.

@da-anda

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

this is the PR that introduced the code that's changed here:
#13566

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

I do know we always want to update stream details

Reading #13566 that seems not always to be the case. Maybe @AkariDN should be given the chance to explain, or suggest another solution to the issue

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

if you want to break #13566, go ahead :)

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

@FernetMenta : From what I get the guy that did #13566 didn't realise that we previously always would update the db with player's stream details so this doesn't break #13566 but simply restores the old logic, still supporting #13566. The only thing is that setting the additional property is a nul-operation now.

@da-anda

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

from what I recall some users didn't want that VP did overwrite whatever was scanned into the DB, because when you think about DVDs and BluRays the stream itself might be 16:9 but the actual movie might be 21:9 and be encoded with black bars. So VP would overwrite 21:9 with 16:9 again, wouldn't it?

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

@FernetMenta : Didn't the "update stream details by default" logic get lost with the whole player/app refactor that was done for v18? That may have confused @AkariDN

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

from what I recall some users didn't want that VP did overwrite whatever was scanned into the DB, because when you think about DVDs and BluRays the stream itself might be 16:9 but the actual movie might be 21:9 and be encoded with black bars. So VP would overwrite 21:9 with 16:9 again, wouldn't it?

Could be, but than we need another mechanism than this since now the cure is worse than the disease. "The old behavior" where you always update the stream details is what you want in the majority of cases and only for a few corner cases you want the "new behavior".

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

Updating stream details in general was dropped by @peak3d because the implementation was crap and resulted in segfaults: #12651
btw: even this old behavior had a condition. In other words stream details were not updated for all cases.

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

Updating stream details in general was dropped by @peak3d because the implementation was crap and resulted in segfaults: #12651
btw: even this old behavior had a condition. In other words stream details were not updated for all cases.

But the segfault can't occur now anymore, since it uses a callback from player, right? So the only issue that remains then is the exclusion of some types (which we could c-p from the old code) and optionally force update those with the property set (?)

EDIT: The relevant piece of code is in this block:

if (!(m_progressTrackingItem->IsDiscImage() || m_progressTrackingItem->IsDVDFile()) || m_pPlayer->GetTotalTime() > 15*60*1000)

@AkariDN

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

Maybe @AkariDN should be given the chance to explain, or suggest another solution to the issue

Stream details for internet streams were not updated in 18a2 so I implemented this logic to allow plugins to save stream details from VP by setting this property to fileitem.

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Maybe @AkariDN should be given the chance to explain, or suggest another solution to the issue

Stream details for internet streams were not updated in 18a2 so I implemented this logic to allow plugins to save stream details from VP by setting this property to fileitem.

But what's the point of only setting the stream details when this property is set? Why not always do it?

@AkariDN

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

Why not always do it?

I didn't know exactly the reason why this old behavior was dropped and not re-implemented. I just wanted to make it work again for plugins without globally changing the current behavior and add more flexibility. It would be better to discuss it with FernetMenta and peak3d.

@arnova arnova added the Type: Fix label Feb 24, 2019

@arnova arnova force-pushed the arnova:always_player_streamdetails branch from 2e05939 to 81ed925 Feb 24, 2019

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

@peak3d / @FernetMenta : I've reinstated the old logic to determine whether we should update stream details. Please let me know what you think. I still believe the new property that was introduced for this can go, right?

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

@arnova have you found the PR which removed the 17.x functionality ?

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

@arnova have you found the PR which removed the 17.x functionality ?

@peak3d : Yes, as mentioned by @FernetMenta : #12651

if (file.GetProperty("get_stream_details_from_player").asBoolean())
/* Always update streamdetails, except for DVDs where we only update
streamdetails if total duration > 15m (Should yield more correct info) */
if (!(file.IsDiscImage() || file.IsDVDFile()) || m_appPlayer.GetTotalTime() > 15*60*1000)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Feb 25, 2019

Member

GetTotalTime is undefined / zero at OnPlayBackStarted

This comment has been minimized.

Copy link
@arnova

arnova Feb 26, 2019

Author Member

@FernetMenta : Good point. Any idea how to proceed with this? Perhaps only drop the GetTotalTime()-part and replace it with the GetProperty() call ?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Feb 26, 2019

Member

For what reason do you place the code into OnPlaybackStarted?

This comment has been minimized.

Copy link
@da-anda

da-anda Feb 26, 2019

Member

would you suggest OnAVStarted()? There all the required info should be ready, right?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Feb 26, 2019

Member

To me the intent described by the comment does not make much sense. Maybe some workaround to skip intro tracks instead of properly detecting the main title. Orignally introduced by @notspiff 8877348
If you are not able to determine the original intent, it is better to drop the condition and save stream details always.

This comment has been minimized.

Copy link
@arnova

arnova Feb 26, 2019

Author Member

Yes, makes sense. Changed it back to what I initially had.

@arnova arnova force-pushed the arnova:always_player_streamdetails branch from 81ed925 to 5f77182 Feb 26, 2019

@arnova arnova force-pushed the arnova:always_player_streamdetails branch from 5f77182 to be1ed70 Feb 26, 2019

@Rechi Rechi changed the title fixed: We should always update stream details from player (fixes #15584) fixed: always update stream details from player (fixes #15584) Feb 26, 2019

@arnova

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

So we merge this "as-is" or is there anything else we need to take care of?

@gate1975mlm

This comment has been minimized.

Copy link

commented Mar 2, 2019

Hello,
I was one of the original people who reported this issue and was just wondering if this will be fixed and when a build will be available to try? Thanks :)

@arnova

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

@arnova I think for the majority of cases this is correct. If you start videos with intros and and stop while playing the into, you'll get the details form the into stored. Maybe it has always been like this and the exclusion was just for DVDs. Personally I don't care much about DVDs because this format is dead anyway.

@arnova

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

@FernetMenta : Yes my idea exactly: dvd is a dead horse. And if "issues" do popup we'll solve those then.

@arnova arnova merged commit 209c6ab into xbmc:master Mar 4, 2019

1 check failed

default Sorry, building this PR failed. Please check the logs for the errors.
Details
@KarellenX

This comment has been minimized.

Copy link

commented Mar 25, 2019

Note to self for future reference...
https://forum.kodi.tv/showthread.php?tid=342336&pid=2838656#pid2838656

It was impossible to save stream details for DVD and Bluray folder before this fix.

@KarellenX

This comment has been minimized.

Copy link

commented Mar 26, 2019

Hello @arnova

Surprisingly this fix does not work for movies saved as DVD folders. Is there a chance you could look at it again? Bluray and ISO's seem to work though.

https://forum.kodi.tv/showthread.php?tid=342336&pid=2838734#pid2838734

Thanks

@arnova

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

@KarellenX : I created arnova@0c4108f which should fix it, but somehow it's not picked up by the skin. I don't have the time at the moment to further look into it, but for others this may be a good starting point.

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.