Review Request: phonon phive core frontend api

Wed Sep 26 20:42:16 BST 2012

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106566/#review19473
-----------------------------------------------------------

Live review at Randa by Harald, Mark and me. Some comments mine, some group conclusions.

core/AudioDataOutput.h
<http://git.reviewboard.kde.org/r/106566/#comment15392>

    I like the idea of deriving this class for advanced use cases. Simple ones (like amarok spectrum visualization) using signals. But signals are probably not appropriate for high frequency events. Your signal would either emit infrequently (once per second), but with bigger datasets or you need a direct callback into the receiver by subclassing it anyway.

    What about sample rate and sample format? You'll need a property to set the format and another to read the rate.

core/BackendCapabilities.h
<http://git.reviewboard.kde.org/r/106566/#comment15394>

    Missing a few: VideoOutput, ...

    How about making this list runtime expandable. Need to think about use cases though.

core/Player.h
<http://git.reviewboard.kde.org/r/106566/#comment15395>

    New name for MediaObject

core/Player.h
<http://git.reviewboard.kde.org/r/106566/#comment15396>

    Should be addAudioOutput and addVideoOutput if you want to keep them separate.

core/Player.h
<http://git.reviewboard.kde.org/r/106566/#comment15397>

    No, should be at the source. Or the video output can tell us. One could be convenience for the other.

core/Player.h
<http://git.reviewboard.kde.org/r/106566/#comment15398>

    Metadata should go in the source!

    Would be nice in Amarok that we can simply map Track to Phonon::Source in the metadata handling.

core/Player.h
<http://git.reviewboard.kde.org/r/106566/#comment15399>

    no. Total can be unknown and it's a one line calculation.

core/Player.h
<http://git.reviewboard.kde.org/r/106566/#comment15400>

    You want to know the required resolution. And no, it's not easy to calculate when it might change!

core/Queue.h
<http://git.reviewboard.kde.org/r/106566/#comment15401>

    Provide some convenience functions for this.
    iterator, index, append, prepend, etc.

core/Queue.h
<http://git.reviewboard.kde.org/r/106566/#comment15402>

    Couldn't I put the same source in multiple times? Which one will you delete?

    Again, convenience functions needed.

    Or will the Queue be set?

core/Source.h
<http://git.reviewboard.kde.org/r/106566/#comment15405>

    s/Stream/AbstractStream

    Shouldn't this just be a QIODevice. Sensible interface, let's not define our own.

core/Source.h
<http://git.reviewboard.kde.org/r/106566/#comment15404>

    Source

core/Source.h
<http://git.reviewboard.kde.org/r/106566/#comment15406>

    No, you just want to take ownership.

    Special case on AbstractMediaStream

core/Source.h
<http://git.reviewboard.kde.org/r/106566/#comment15407>

    To be examined, but me likes.

core/VideoDataOutput.h
<http://git.reviewboard.kde.org/r/106566/#comment15408>

    Use for simple usecases, derive for advanced stuff.

core/abstract/AbstractVideoOutput.h
<http://git.reviewboard.kde.org/r/106566/#comment15410>

    Add same effect (object), what happens? Ordering?

    Could make this a QSet

core/effects/SubtitleEffect.h
<http://git.reviewboard.kde.org/r/106566/#comment15413>

    Get the list of subtitles from the source.

core/effects/SubtitleEffect.h
<http://git.reviewboard.kde.org/r/106566/#comment15412>

    properties needed.

- Bart Cerneels

On Sept. 25, 2012, 11:06 a.m., Harald Sitter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106566/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2012, 11:06 a.m.)
> 
> 
> Review request for Amarok and Phonon.
> 
> 
> Description
> -------
> 
> phonon phive core frontend api
> 
> 
> Diffs
> -----
> 
>   core/AudioDataOutput.h PRE-CREATION 
>   core/AudioDataOutput.cpp PRE-CREATION 
>   core/AudioOutput.h PRE-CREATION 
>   core/AudioOutput.cpp PRE-CREATION 
>   core/BackendCapabilities.h PRE-CREATION 
>   core/BackendCapabilities.cpp PRE-CREATION 
>   core/OutputEffect.h PRE-CREATION 
>   core/OutputEffect.cpp PRE-CREATION 
>   core/Player.h PRE-CREATION 
>   core/Player.cpp PRE-CREATION 
>   core/Queue.h PRE-CREATION 
>   core/Queue.cpp PRE-CREATION 
>   core/Source.h PRE-CREATION 
>   core/Source.cpp PRE-CREATION 
>   core/VideoDataOutput.h PRE-CREATION 
>   core/VideoDataOutput.cpp PRE-CREATION 
>   core/abstract/AbstractAudioOutput.h PRE-CREATION 
>   core/abstract/AbstractAudioOutput.cpp PRE-CREATION 
>   core/abstract/AbstractMediaStream.h PRE-CREATION 
>   core/abstract/AbstractMediaStream.cpp PRE-CREATION 
>   core/abstract/AbstractVideoOutput.h PRE-CREATION 
>   core/abstract/AbstractVideoOutput.cpp PRE-CREATION 
>   core/core.pro PRE-CREATION 
>   core/effects/SubtitleEffect.h PRE-CREATION 
>   core/effects/SubtitleEffect.cpp PRE-CREATION 
>   core/five_global.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106566/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Harald Sitter
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20120926/f8a96405/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia