[libbluray-devel] first patch-set

[hpi]

2011/1/23 Erik Van Grunderbeeck <erik at arawix.com>:
> Patch set for graphics controller :

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.

> Graphics controller
>
> 1) Fix to clear the page when new items show up (ege in case of animations,
> they are otherwise overdrawn).

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 ?

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.

> 2) Fix for some bluray that have startup bogs to start loading second pages
> (where the action flag is used).

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).

> 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 ?

> 4) Return value if a user input selection is valid.

OK, I assume you're planning to add that to API level too :)

> I have in pipeline:
>
> 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.

> 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.

> A fix to return better pts in searching.

Maybe j45 should comment on this one.

> Question2: some pgs graphics need to active object to stay active. Currently
> at the end of the vm loop, its de-activated to continue reading data. Any
> thoughts on how to handle that better?

Do you mean HDMV title object is suspended when playlist playback
starts, or it is resumed (too early) when end of title is reached ?

---------------

>+    // check if we had a startup bog, preparing the page (ege some universal BR's like BG-the plan)
>+    // note that this may not be needed once the animations work correctly...
>+    // 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)
>+    if( (0 == ifound) &&
>+        (page->num_bogs == 1) &&
>+        (page->default_selected_button_id_ref == page->default_activated_button_id_ref)
>+    )

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
...
If that initialization page uses selection timeout, the check should
use also timeout pts and be implemented so that it is triggered for
any reason gc_run() is called for. But that seems not to be used here,
as button object has also auto_action flag set.

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.

>+    // 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 ?

>+        /* 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.

>@@ -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).

--> 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]