Review Request: phonon phive core frontend api

Harald Sitter sitter at kde.org
Fri Oct 12 16:16:15 UTC 2012 


> On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote:
> > core/Player.h, line 52
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line52>
> >
> >     no. Total can be unknown and it's a one line calculation.
> 
> Matěj Laitl wrote:
>     But Phonon may know the length better than we in Amarok. I would personally like to use some kind fo remainingTime signals to implement bounded playback in Amarok.
> 
> Bart Cerneels wrote:
>     Phonon would internally do the same calculation (remaining = total - elapsed). If you add the convenience function, shouldn't you also add the notify signal, etc?

iff we grow a remainingTime thing it'd be a property with read accessor and signal... question is whether we want that or not

since the original draft I however came to think that perhaps that may be a very useful thing. all player type applications will have some sort of remainingtime feature so simply having a signal that they can attach a widget or qmlitem to seems like a good idea.


> On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote:
> > core/Player.h, line 53
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line53>
> >
> >     You want to know the required resolution. And no, it's not easy to calculate when it might change!
> 
> Matěj Laitl wrote:
>     I'd like to ditch ticking from Amarok code in favor of some kind of remainingTime mechanism.

please elaborate.

sandsmark raised the idea of introducing spatial ticking ... i.e. you tell phonon that your visual element for progress display (e.g. progressbar) is N pixels long and phonon tries to emit ticks such that you would get one signal per pixel to update. is that what you are thinking about?


> On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote:
> > core/Queue.h, line 26
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87104#file87104line26>
> >
> >     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?
> 
> Matěj Laitl wrote:
>     > Or will the Queue be [a] set?
>     
>     Cannot be, set doesn't preserve order and we really need it for a .. queue.

best talk this over with mr. t.

as I see it if you were to insert the same source twice you'd still need it to be different source objects... that of course is a bit silly if you make a queue of ABABABABABAB. BUT this case IMHO only applies to players. in a scenario where a more simplistic consumer wants to use the queue they may easily be able to ensure that no duplicated sources are queued. so I would not generally dismiss this particular interface here. instead, if the queue were to grow iterator support we could have an iterator based overload to address multi-source dequeuing.

the interface at hand would, due to the name, only dequeue the first entry of source. iff there is a use case for massdequeing a dequeueAll function may be nice.


> On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote:
> > core/Source.h, line 19
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87106#file87106line19>
> >
> >     s/Stream/AbstractStream
> >     
> >     Shouldn't this just be a QIODevice. Sensible interface, let's not define our own.

it could. it would however be somewhat ugly as we only need like 5 functions and thus the QIOD impl would be limited in terms of a QIOD. also what we are already seeing is that not every QIOD fits our needs. like a Qnetworkreply. it is a QIOD but has additional functions to determine the end of the reply (i.e. network transfer may still be in progress, but currently available buffer is completely read already). so even right now you'd not be able to stuff a QNR into the AMS magic and assume it works unless the QNR has completely finished transfer. now if we were to drop the AMS crap in favor of a QIOD you'd have to write a QIOD to wrap around QNR (which is a QIOD) to stream from that....

perhaps you could drop a mail to the list for discussion, but I am really not a fan of this.


> On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote:
> > core/abstract/AbstractVideoOutput.h, line 16
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87114#file87114line16>
> >
> >     Add same effect (object), what happens? Ordering?
> >     
> >     Could make this a QSet
> 
> Matěj Laitl wrote:
>     > Could make this a QSet
>     
>     No, effect order matters and QSet doesn't preserve order.

adding the same effect twice will prolly end up failing an assert ... I am reasonable certain that vlc does not support this so...

and yeah, order matters... speed up before eqalizer for example


- Harald


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


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/amarok-devel/attachments/20121012/22c2d934/attachment.html>

More information about the Amarok-devel mailing list