[prev in list] [next in list] [prev in thread] [next in thread] 

List:       haiku-development
Subject:    [haiku-development] Aw: Re: [Request for protest] Media Kit: Extend media_header struct
From:       "Colin_Günther" <ColinG () gmx ! de>
Date:       2014-07-24 15:15:43
Message-ID: trinity-b74a606e-2d0e-4a17-a3d6-a0e4ec28cf45-1406214943238 () 3capp-gmx-bs36
[Download RAW message or body]


Hi  Jérôme,

> There is a b_media_format_changed for this need, isn't there?

thanks for pointing this one out. I didn't have that on my list before, so I did some \
research and had a look on how B_MEDIA_FORMAT_CHANGED is intended to work (see \
further below = Research results =).

TL;DR: In conclusion I'll keep sticking to my original proposal as it is, because the \
B_MEDIA_FORMAT_CHANGED is neither intended (no media_source and media_destination \
relationship in the BMediaDecoder class) nor suited (more processing overhead, due to \
involving the media_server in message relaying) for the particular use case outlined \
in my proposal. 

In = Code Path outline of a BMediaDecoder Format Change = further below I am going to \
be a little bit more elaborate about why I think that B_MEDIA_FORMAT_CHANGED is \
neither intended  nor suited for the particular use case in BMediaDecoder.

@Jérôme: I hope I did understand you suggestion right and inferred your intentions \
correctly. If you like to be more specific on how you would see \
B_MEDIA_FORMAT_CHANGED fit into the BMediaDecoder feel free to outline it here :)


= Research results =

As far as I understand it, this notification is sent by the media_server, when there \
is a listener registered interested in receiving B_MEDIA_FORMAT_CHANGED \
notifications. Although the media_server sends those notifications, their origin is \
always a BMediaNode, that tells the server that there was a media format change.

A B_MEDIA_FORMAT_CHANGED notification contains three values [1] that are of interest \
for the notification listener: 1. media_source
2. media_destination
3. media_format

Currently the BMediaDecoder class, doesn't need and doesn't contain a reference to \
media_source and media_destination variables. And as far as I can tell, this is by \
design, as you should be able to decode data chunks without the need of instantiating \
any BMediaNode.

So issuing a B_MEDIA_FORMAT_CHANGED notification from the BMediaDecoder class would \
either involve changing the scope of BMediaDecoder to allow storing of media_source \
and media_destination or extend the notification system to allow registering for \
notifications from BMediaDecoders, too, without the need of storing a media_source \
and media_destination.


= Code Path outline of a BMediaDecoder Format Change =

== Example 1: Proposal ==
The following code path outline demonstrates the execution flow of a format change \
detection and propagation - using my proposal as basis - by taking the example of the \
dvb.media_addon. This will be used to help illustrating my hesitation to implement \
any notification scheme in the BMediaDecoder to inform clients about a format change:

1. BMediaDecoder returns media_header with the new fields (e.g. pixel_width_aspect) \
of the proposal filled out.

2. The DVBMediaNode compares returned media_header fields with values of a previous \
call to BMediaDecoder::Decode() [2].

3. DVBMediaNode detects a change that is relevant for continuing its processing and \
messages the media_destination (typically a BBufferConsumer) about the change in \
format via BBufferProducer::ChangeFormat() [3]

4. The BBufferConsumer than receives the message and calls its concrete \
implementation of the virtual method BBufferConsumer::FormatChanged() [4, 5].

=== Conclusion ===
+ The DVBMediaNode can decide on itself what it interprets as format change and what \
not. + No performance penalty for a client that isn't interested in detecting the \
format change as it just omits comparing media_header values. + Legacy code still \
                works, as the media_header structure keeps the same size.
- We loose some free space in the media_header structure, due to adding new fields. \
So we do limit our flexibility in terms of reducing the number of fields we can \
                potentially introduce.
- The DVBMediaNode has to decide on its own what it interprets as format change and \
what not, thus making it more complex.


== Example 2: Notification based ==

Now let's look at a possible code path when introducing a B_MEDIA_FORMAT_CHANGED \
based solution. This solution assumes that you can register for events emitted by \
BMediaDecoders, too, and that the BMediaDecoder doesn't no anything about the \
recipients of its decoded media data, meaning it doesn't store any reference to a \
media_source or media_destination.

1. DVBMediaNode registers for B_MEDIA_FORMAT_CHANGED notifications at the \
media_server for its BMediaDecoder.

2. BMediaDecoder detects a format change on its own and informs the media_server \
about it.

3. media_server informs the DVBMediaNode about the format change.

4. DVBMediaNode messages the media_destination (typically a BBufferConsumer) about \
the change in format via BBufferProducer::ChangeFormat().

5. The BBufferConsumer than receives the message and calls its concrete \
implementation of the virtual method BBufferConsumer::FormatChanged().

Notice that by allowing clients to register for BMediaDecoder based notifications the \
B_MEDIA_FORMAT_CHANGED notification now becomes an intended use case. This just \
demonstrates the flexible nature of software where nothing is set in stone and where \
just claiming that an API is not intended for this or that isn't really an argument. \
But you will see, that even when B_MEDIA_FORMAT_CHANGED would have already been \
intended for use with BMediaDecoders, it still wouldn't suite the use case very well, \
due to the added overhead of involving the media_server in relaying the \
B_MEDIA_FORMAT_CHANGED notification.

=== Conclusion ===
+ Reuse of existing infrastructure, after extending it for the new use case
+ Several clients could register for format change notification of one BMediaDecoder \
(this is purely theoretically for me right now, as I don't now any real life \
application where this would made sense, but I have an open mind ;) + DVBMediaNode \
doesn't need to decide what counts as a format change and so keeping it simpler at \
least lines of code wise. + DVBMediaNode can opt-in to decide what counts as a format \
change by evaluating the format change notification and deciding whether to propagate \
                the message or not.
- Extending the current infrastructure means more implementation, test and \
                documentation work compared to the proposal solution
- Longer (potentially slower) code path, because now the media_server is involved in \
relaying the format change message, too. o Don't know what this means in terms of \
backwards compatibility with legacy code, would have to do more research here. But at \
least I can say, that coming up with an is compatible / is not compatible answer is \
more involved then for my proposal ;)

== Summary ==
I hopefully demonstrated that going my proposed way (Example 1) (and when I write \
-my- please keep in mind that it is basically the solution of Marcus Overhagen, so he \
should take the credits for it) is an acceptable route and that a notification based \
way (Example 2) isn't giving any real life value at the moment.

> 
> Bye,
> Jerome

Ciao
Colin

= References =
[1] B_MEDIA_FORMAT_CHANGED values: \
http://cgit.haiku-os.org/haiku/tree/src/kits/media/Notifications.cpp#n195 [2] \
Detecting format change: \
http://cgit.haiku-os.org/haiku/tree/src/add-ons/media/media-add-ons/dvb/DVBMediaNode.cpp?id=hrev47565#n1780
 [3] Informing media_server about format change: \
http://cgit.haiku-os.org/haiku/tree/src/add-ons/media/media-add-ons/dvb/DVBMediaNode.cpp?id=hrev47565#n1811
 [4] Declaration of virtual method BBufferConsumer::FormatChanged(): \
http://cgit.haiku-os.org/haiku/tree/headers/os/media/BufferConsumer.h?id=hrev47565#n98
 [5] Example implementation of virtual method BBufferConsumer::FormatChanged() (in \
VideoNode class of TV app): \
http://cgit.haiku-os.org/haiku/tree/src/apps/tv/VideoNode.cpp?id=hrev47565#n305


[prev in list] [next in list] [prev in thread] [next in thread]