[libbluray-devel] first patch-set

[hpi]

2011/2/3 Erik Van Grunderbeeck <erik at arawix.com>:
> A)
>>It would be easier to discuss and track each change if you could split
>>the patch and send each fix as separate mail. You can commit changes
>>locally and generate patches with "git format-patch origin" command.
>>That produces patch with description from each commit. Such patches
>>can be directly imported and pushed, and git will record correct
>>author.
> I will. Sorry for that. I was not sure on what was going to be ok, and just
> wanted to have some thoughts.
> I should have made seperate patches. Will do (I need to get a better grip on
> Git, I am to much used to SVN).

git-format patches are not requirement, it just makes applying patches
easier and faster :)

> B)
>>Are there cases where new objects do not overdraw previous ones
>>completely? Clearing page here (every time page is rendered) prevents
>>further optimizations to page drawing - it could render only changed
>>objects. It may also cause some visible flickering ?
> Yes. I have one disk where a selection makes a smaller rectangle over a
> previous one (also in the case of popup menu in popup menu).

So it does not change page, just enables/disables some buttons ? I
have some similar menus, but those always change page when "sub-menu"
is opened / closed, so I haven't seen the problem :)

> The graphics controller now just keeps on pushing data. My change makes it
> more a real page handling:
> Once you get a clear, all objects that are pushed should be drawn, until the
> next clear.
>
> Flickering/optimalisations are left to the control callback (ege if it knows
> its clear, don't reclear).

Currently application does not not know when page is complete, so
flickering can't be avoided. Application can't know if page was
cleared for redraw or because of closed menu ; it also does not know
when page is complete and ready for display. This should be fixed
somehow ; maybe with some flag in the struct passed to application
callback (?).

> C)
>>If there are objects that are hidden at some point, maybe we should
>>draw transparent object there. If this is the case, it should be OK to
>>clear page here as a temporary solution.
> Thats the other issue: alpha control. If you overdraw, you have to make sure
> the objects are presented in the correct order, otherwise the alpha overlay
> will look all messed up.

Alpha control shouldn't be an issue. New objects are supposed to
overdraw (replace) previous data in the graphics plane. Alpha channel
should be used only when mixing graphics plane to video frames.
When object is drawn, it replaces all pixels in the rectangle. Two
object from same page can't overlap, so drawing order should not
matter.

When there's no object for button, transparent background should be
rendered. Currently this is not implemented.

> D)
>>There must be unhandled case or bug somewhere if it misses auto action
>>flagged buttons. Auto action should be already implemented, I even
>>tested it with similar initialization pages. Looks like
>>selected_button_id is invalid when entering _render_page(). Maybe
>>resetting selected button or call to _update_enabled_button() is
>>missing from somewhere. Can you add some debug output to see what
>>button is selected, and what buttons are in the BOG ? If it is set to
>>wrong button, it could be also interesting to know where the invalid
>>value is written to register (when showing the menu first time).

I finally figured this out. Now selected button is always
checked/updated when menu page changes or menu is opened.

I've committed this change, but it is not very well tested. I hope
there are no unwanted side-effects :)

> Once I get home, will do. I have looked at this for some time, but didnt see
> a pattern in the id's.
>
> It seems a lot of Universal DVD's use the same framework. They have one page
> with a bog at start. That triggers the next page, that contains the actual
> selection.
>
> The "hidden" bog/button on the first page doesnt seem to have some
> autostart/autotrigger connected to it that I could find. (See also you
> remarks on timeout later)

The button should have auto_action_flag set ?

> I'll log some of them.
>
> E)
>>> 3) Fix for interactive popup menu's that go back to first menu when
> playing
>>> a video is done.
>>Probably same bug as in 2). The code is missing some initializations or
> checks.
>>Could be also IG command sequence messing up too early restored title
>>object state ?
>
> Example what happens:
>
> example
>
> Page: A (id 0)
> Button: Play
>
> Select play
>
> Page: B
> Buttons:
> Play -> Play main movie (id 0)
>     -> Play extended version (id 1)
>
> Select "Play extended". Movie feature plays. At the end of the playlist,
> Page A is shown by the controller because of the VM. But the active id is
> still (id 1 from page B). Which is not present on Page A.

See D)

> F)
>>> 4) Return value if a user input selection is valid.
>>OK, I assume you're planning to add that to API level too :)
>
> Yep ;) .

Committed

> G)
>>> Fixes to create exported symbols for a windows dll. Not sure what you
> guys
>>> plans are there
>>That's welcome. Also all other fixes to build libbluray for Windows.
>
> I have some more. I am currently using the Intel compiler (v11) to compile
> the code. I am not sure what you want to do there. Theres a way to generate
> automatic exports also *but not sure on how that will mess what the java vm
> later*. Not sure whats prefered.

How it selects exported functions when exports are auto-generated ?
That would be easiest way to go ... Does it require some attribute for
each exported (or hidden) function, or can the functions be selected
by some pattern mask (like bd_*) ?

If it requires some __dllexport attribute, we can add BD_PUBLIC to
functions in bluray.h and then conditionally include attributes.h when
building the library, and define BD_PUBLIC to empty otherwise.

List of exported functions should be easy to generate with grep / sed
too. So, if autogenerating the list requires some compiler-specific
magic, building our own script to collect the list could be most
portable.

> H)
>>> Some changes when the intel compiler is used (it doesnt like ##__VA_ARGS,
>>> but accepts #__VA_ARGS).
>
>>Would it accept C99 style variadic macros ?
>>    #define TRACE(...) debug(flags, __VA_ARGS__)
>>It would be simpler if we could use single macro for different compilers.
> I Need to check. It is also wrapped in a bad defines now. I was still
> messing around. I was thinking it might be nicer to put them all in one file
> too ;)
>
> I)
>>> A fix to return better pts in searching.
>>Maybe j45 should comment on this one.
> Problem is now that one never gets the final pts points of a playitem,
> always the minimum maximorum. I am not sure on how to handle it better
> though.
>
[...]
>
> K)
> <cut some stuff>
>>page->default_activated_button_id is used when user selection
>>timeouts. If it is set to valid button, the button should be activated
>>when user selection timeouts (selection_timeout_pts is reached). But I
>>belive selection timeout can't be used here: it should be set to start
>>of presentation to trigger action immediately. It wouldn't work as
>>expected as it applies to whole composition, not only to single
>> (initialization) page. Unless none of the other pages use the feature
>
> Ah it all starts to make sense now. Thats probably the correct way to do it.
> Problem is, I have at least one disc (Galactica The plan) that doesnt have a
> valid default_activated_button_id (set to 0xffff).
>
> Which brings me the next question: how does the library get the current
> playback pts?

It is up to the application to provide current time. There's no way
knowing it otherwise.

> And is it the pts, or the dts, or the 4 byte extra time data
> at the start start? :)

Current player SCR (~ currently showing video frame pts / audio frame pts).

> Dont say from the pgs stream:
>
> I have one disc that works like this: 1 PGS stream, one video stream, one
> audio. Video has 3 frames (at start), then 15 seconds of music. Disc has 2
> buttons (language selection). The timeout is obviously at the end of the
> audio, but since we dont demux everything (and probably shouldnt), how will
> we know the pts/dts for timeout?

(Timers are not yet implemented)

Player SCR should still be running unless playback is paused.
Currently player can update PSR_TIME by calling bd_vk_key(bd,
BD_VK_NONE, current_time) at regular intervals (maybe once in 40ms,
definetely not more often). Note that this won't execute the action at
timeout unless bd_read_ext() is also called. I think this should be
changed so that button actions are always executed immediately.

It might be good idea to provide special function for updating current
time (that would just update PSR_TIME value). Graphics controller and
HDMV VM could then hook to PSR_TIME change callback when there is
active timer or animation.
There are at least two types of timers: Navigation timer can be set in
HDMV code, and IG menus have different timeout fields.

> L)
>>Should be OK as temporary fix if (page->default_selected_button_id_ref
>>== page->default_activated_button_id_ref) is removed ; but I would
>>still prefer finding out why selected button is incorrect, as the bug
>>will show up in other situations too.
> Me too. I put page->default_selected_button_id_ref ==
> page->default_activated_button_id_ref in there as an extra check.
>>+    // this also fixes the case where the PSR register with the selected
> button id is not in
>>+    // the current page anymore (which happens after a selection in a sub
> menu, a play, and then a jump back to main menu)
>>But only when composition has single BOG initialization page 0 ?
>
>
> Nope. See above (@ E) )
>
> M)
>>>+        /* sometimes null, not sure why*/
>>>+        if(gc->enabled_button == NULL)
>>>+           continue;
>>>+        valid_id = gc->enabled_button[ii];
>>There's no point to run the whole function without this information,
>>looks like it is called before IG composition has been completely
>>decoded. gc_decode_ts() and gc_run() should be fixed instead (the
>>problem will show up in other places too). Maybe it is enough to check
>>for complete display set in gc_run():
>>-     if (!gc || !gc->igs || !gc->igs->ics) {
>>+     if (!gc || !gc->igs || !gc->igs->complete ||  !gc->igs->ics) {
>         TRACE("gc_run(): no interactive composition\n");
>>Note that there's no proper locking in graphics controller so this
>>could be threading issue too.
>
> Ok. I will add that and see if I can find the disc again. Wanne leave it in
> to prevent a dump?

Couple of recent commits should have fixed this.

> N)
>
>>@@ -712,22 +755,27 @@ static void _mouse_move(GRAPHICS_CONTROLLER *gc,
> unsigned x, unsigned y, GC_NAV_
>>
>>         BD_PG_OBJECT *object = _find_object_for_button(s, button,
> BTN_NORMAL);
>>         if (!object)
>>-            continue;
>>+        {
>>+            /* try selected then. sometimes normal is -1*/
>>+            object = _find_object_for_button(s, button, BTN_SELECTED);
>>+            if (!object)
>>+               continue;
>>+        }
>
>>OK. Looks like it should be enough to just fetch BTN_SELECTED object,
>>as it is checked later anyway (we select button only if it can be
>>selected).
>
> Agreed
>
> O)
> --> I'd suggest following change:
>
> @@ -712,22 +755,27 @@ static void _mouse_move(GRAPHICS_CONTROLLER *gc,
> unsigned x, unsigned y, GC_NAV_
>
> -        BD_PG_OBJECT *object = _find_object_for_button(s, button,
> BTN_NORMAL);
> +        BD_PG_OBJECT *object = _find_object_for_button(s, button,
> BTN_SELECTED);
>         if (!object)
>             continue;
> ...
> -         // can button be selected ?
> -         if (!_find_object_for_button(s, button, BTN_SELECTED))
> -            return;
>
> [all objects of same button / BOG must have same dimensions]

Committed.