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

Decoderfilter implementation #15744

Merged
merged 1 commit into from Mar 19, 2019

Conversation

@peak3d
Copy link
Contributor

commented Mar 14, 2019

Description

Implementation of a xml configuration file to control usage of platform decoders.

By adding xbmc/media/decoderfilter to your platform subdirs.txt file, kodi builds a common CDecoderFilterManager implementation.

You as a platform maintainer now have to instanciate this or an derived class of CDecoderFilterManager , add all existing / relevant platform decoders, and register this interface in ServiceBroker (at kodi / windowing initialization time, once).

In your decoder initialization part you access the DecoderFilterManager through ServiceBroker and can check if a selected decoder should be used for playback by passing the StrreamInfo hints.

CDecoderFilter's members for validateing / serializing can be overwritten if your platform has conditions which differ from the common implementation.

Serialization is already implemented and leads to [userdata]/decoderfilter.xml file.

The implementation should be flexible enough to add GUI settings handling, regardless if conditions differ between platforms. GUI settings is not part of this PR, if I find time in the future, I'll add it.

For now experienced users can manually edit the decoderfilter.xml file.

Default XML implementation file format:

<decoderfilter>
  <filter>
    <name>[identifier, e.g.  OMX.google.aac.decoder]</name>
    <allowed>[true/false], general condition</allowed>
    <stills-allowed>[true/false], condition for streams which contain still frames</stills-allowed>
    <dvd-allowed>[true/false], condition for streams which origin from dvd</dvd-allowed>
    <min-height>[#pixel] minumum stream height to allow this decoder</min-height>
  </filter>
  <filter ..... />
</decoderfilter>

This PR builds / uses the implementation only for Android

Motivation and Context

tons of tickets regarding not working / half working platform decoders

How Has This Been Tested?

Android,

  • Start kodi
  • manually enable / disable a decoder by editing the file [userdata]/decoderfilter.xml
  • restart kodi, validate if the changed setting has effect

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)

#include "media/decoderfilter/DecoderFilterManager.h"


This comment has been minimized.

Copy link
@lrusak

lrusak Mar 14, 2019

Contributor

extra line

@lrusak

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

This is a great idea!

I don't really understand though why you went for the xml route instead of just hooking up the GUI to use a blacklist for the decoders. It seems to me like you only need one method or the other and the GUI approach would be more useful IMO.

@lrusak lrusak changed the title Decoderfilter implementetion Decoderfilter implementation Mar 14, 2019

@peak3d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

I changed the initial comment, there you can see that things are sometimes more complex as they seem on the first look. Goal is a.) to be flexible for other platforms / decoders (maybe include color formats / codec levels) and b.) to be able to use our setting implementation to easily display them in our GUI if needed. XML is currently the format our GUI setting is based on


std::string codecname = codec_info.getName();
uint32_t flags = CDecoderFilter::FLAG_GENERAL_ALLOWED | CDecoderFilter::FLAG_DVD_ALLOWED;
for (const char **ptr = blacklisted_decoders; *ptr && flags; ptr++)

This comment has been minimized.

Copy link
@lrusak

lrusak Mar 14, 2019

Contributor

why not make blacklisted_decoders a std::array<std::string> and use a ranged based for loop?

This comment has been minimized.

Copy link
@peak3d

peak3d Mar 14, 2019

Author Contributor

Pls. read again the initial (edited) post, its not only a blacklist.

Edit: nvm. Its copy/paste from initial implementation. you can also make a std::set and have binary search, but I was lazy here and used what was there.

Edit2: But question here again: why std::string for constant strings, which get malloced on the heap if I use std::string?

@enen92 enen92 added the Wiki: Needed label Mar 14, 2019

@pkerling

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

@peak3d It's just a small thing, but could you in general align the pointer/reference modifiers to the type instead of the variable? i.e. const std::string& a instead of const std::string &a

@peak3d peak3d force-pushed the peak3d:hwdecoder branch from 7e7ecb3 to 21912bb Mar 18, 2019

@peak3d peak3d merged commit 76db73d into xbmc:master Mar 19, 2019

1 check passed

default You're awesome. Have a cookie
Details

@peak3d peak3d deleted the peak3d:hwdecoder branch Mar 19, 2019

@ksooo

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Merged, alltough flagged as "v19"?

@@ -127,11 +128,15 @@ class CServiceBroker
static void RegisterAppPort(std::shared_ptr<CAppInboundProtocol> port);
static void UnregisterAppPort();

static void RegisterDecoderFilterManager(CDecoderFilterManager* manager);

This comment has been minimized.

Copy link
@ksooo

ksooo Mar 19, 2019

Member

why is there no unregister method? is the memory leaked intentionally?

@anssih anssih referenced this pull request Mar 19, 2019

Open

ALSA cards missing in Kodi's audio device list #15757

1 of 7 tasks complete

@ksooo ksooo added v18 Leia and removed v19 Matrix labels Mar 20, 2019

@ksooo ksooo added this to the Leia 18.2-rc1 milestone Mar 20, 2019

@peak3d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@ksooo K18 / K19 the PR was initially created for K18, but it seems that I forgot to set labels :-(
Register is just setting a pointer var in ServiceBroker.
In this implementation the creator has ownership of DecoderFilter (for android it is WinSystem) and destroys the DecoderFilter (beside Registering a nullptr in ServiceBroker) on UnInitialize.

There are many ways which lead to rome (ownership ServiceBroker e.g.), or smart_ptr.
I'm open for discussions if it should be implemented differently, for me it was important, that other platforms are not affected at all.

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.