[libbluray-devel] first patch-set

[Erik Van Grunderbeeck]

Sorry for delay in replying on other email. I am checking some stuff you
suggested, work kicked in on a project, and I am messing around with my
kinect ;)

Seeing my rereading, I probably should split my emails too. Thanks for the
extensive review btw.

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

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

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

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.

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

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)

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.

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

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.

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.

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

when playlist playback starts. It falls back to my animation question, but
you answered that in another email. Ignore for now until I look better at
the stream demuxing.

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? And is it the pts, or the dts, or the 4 byte extra time data
at the start start? :)

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?

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?

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]

Erik