[vlc-devel] Re: libcddb added, playlist interface - disappointing

[Derk-Jan Hartman]

Combining several of your messages here, rocky. It's gonna be a long  
read :)

On 01 dec 2003, at 02:28, R. Bernstein wrote:
> Sigmund Augdal writes:
>> This information is nearly never parsed in vlc so the wxwindows gui  
>> have
>> decided to hide that column.
>
> I'm not sure I understand you. Do you mean that (input) modules rarely
> set the duration parameter correctly and even though one has to pass
> something, the wxwindows therefore GUI decided not to show it?

Only the playlist parser sets this value i think. It was added and the  
idea was somewhat abandoned. So because there was a column that didn't  
do anything, this column was removed, to not confuse users, while the  
technology remained in place, so that if someone wanted, he could pick  
it up again.

> If so,
> we have a bootstrap problem - why should someone working on module do
> the extra work to figure this out (as I just did) to find that the
> value isn't going to get shown? If one provides and API and requires a
> parameter to be passed, I think user interfaces should be the first to
> use it and not wait for the majority of modules to get the value
> correct.  Or get rid of the duration parameter and field.

Reading the other stuff you wrote after this as well, I have to draw  
the following picture (don't be offended by any of it and just keep on  
reading).
You sometimes seem to expect us to work to an endgoal. We have a  
slightly different approach. We have too much time on our hands and  
like VLC. Sometimes we run into stuff we don't like in VLC, and we fix  
it. We are not actively pursuing to become the best player out there.  
It's nice if we ever get to the point where we are the best, it's just  
not our primary goal, we are only being selfish.

You think extremely user oriented. Which is really good. Problem is  
that many of us often think technology oriented and platform  
independent. It are VLC's main selling points and we need to keep that  
alive. The user point of view is second to this.
Interfaces are platform dependent. You always need to keep working on  
it while VLC matures. A Interface in VLC can never really become  
something until the core is somewhat finished. Steps like redesigning  
the playlist system happen every day, and interfaces need to be  
maintained for that. I for instance spend more time bringing the  
interface back up to date or making it work again, then i actually have  
time to be innovative with it. I hate that. It's maintenance work.   
That is why nooone on the team truly likes to work on building the  
interfaces.

>> The gtk interface on the other hand haven't
>> been updated since this was included in the api and just puts "no  
>> info"
>> reguardless. I have a patch for the gtk interface to set this  
>> properly, and
>> could commit it if anyone wants it.
>
> Yes, I'd appreciate it if you commit it. Or remove the gtk interface
> altogether - but let's not leave an interface that you know has
> problems and you know how to easily solve.

That's too much work. The problem about removing stuff from the  
distribution is that they guaranteed will never be fixed. I prefer too  
lower there priority until someone decides too fix it. And we should  
work with the packagers to have them disable those interfaces and force  
the wxwindows one onto users. If an interface is so broken it is no  
longer able to play a file, only then I'm for removing the interface.

> In the wxwindows case it is just a
>> matter of uncommenting a few lines.
>
>> From what I could see, I have a feeling it's a little more than
> that. So I'd again greatly appreciate it if it were done.
>
> But to do the job properly there should be a routine that converts
> seconds to a nice hour/minute/second string. In
> modules/access/cdda/access.c, I have this:
>
> static
> char * secs2TimeStr( int64_t i_sec )
> {
>   int h = i_sec / 3600;
>   int m = ( i_sec / 60 ) % 60;
>   int s = i_sec % 60;
>   static char buffer[20];
>
>   snprintf(buffer, 20, "%d:%2.2d:%2.2d", h, m, s);
>   return buffer;
> }

Add it.. :) no clue where though.
>
> which in turn is derived from code in modules/demux/mkv.cpp. If such a
> routine already exists or gets added, it would great to remove the
> duplication of code.

indeed.


>> Look at input_InfoCategory() and input_AddInfo() here:
>> http://developers.videolan.org/vlc/vlc/doc/doxygen/html/input__ext- 
>> plugins_8h.html#a28
>> http://developers.videolan.org/vlc/vlc/doc/doxygen/html/input__ext- 
>> plugins_8h.html#a29
>> These will show up in View->File info... and this is the right place  
>> for
>> such information in my opinion.
>
> Okay. That's what I've done. Thanks!
> I know others have problems with this, will comment on that in turn...

Cool, cddb is cooooool. great work.

>
>> I forgot to say that if nobody disagrees, I would enjoy to do this.
> My take is that if you feel you have a better way, I'd really
> appreciate it if you worked on it and shared it.
>
> The problem I think we're having is not that there are too many ideas
> but that there's an awful lot that needs to be done especially in the
> user-interface area.
There is a lot to be done in creating a 'final' core for VLC first.  
Only then we will have the opportunity to build great interfaces.


>> We could also then have functions, like
>> playlist_AddInfoToCurrentItem_ThisNameSucks(), that would allow to get
>> rid of the find/release of p_playlist that we have almost everywhere  
>> in
>> the code

The problem is the amount of locking and stuff that goes into this. You  
really need to be careful about this. BTW. what if at some point (and i  
know fenrir has been working on this) VLC supports multi-input? In  
theory at one time you could have two playlists/controllers and two  
playing files. see later on.

> Yep, probably like yourself I dislike the vlc_object_find( p_xxx,
> VLC_OBJECT_PLAYLIST, FIND_ANYWHERE ) and then accessing the internal
> structures that. And yes it is sad to see this in a number of modules.

I don't. It's how VLC works. It's how it became as modular as it is.
Now everything still uses FIND_ANYWHERE again, in above mentioned  
hypothetical case this would become FIND_PARENT i guess.
It's not the object_find that is the problem. it is the lack of  
accessor methods in several areas of the playlist code that is  
annoying.

> Right now there's no alternative that I can see. But if someone offers
> a better solution, I'll be happy to change the code I've added for  
> this.

Redesigning the playlist system has been on the list since at least a  
year now. Noone likes to burn it's fingers on it. It's working and  
that's all that mattered so far. Now we finally start to see demand for  
new uses of a playlist system, so I think now it will finally happen as  
well.

> Derk-Jan Hartman writes:
>> I agree on this approach. It seems way more logical to store
>> information concerning a file in a structure that is part of a
>> playlistitem then to store it at the input level.
>
> In the first sentence I thought you agree with Sigmund (as I do). But
> from the rest I infer you think along the lines of Zorglub.

Zorglub thinks about using a separate infos structure. Dnumgis and I  
are of the opinion that if you have such a structure already in place,  
that also holds information about a playlist_item, why not combine the  
two. Move the info structure from input to playlist_item (that way you  
can even keep the info after it is played, which is actually kinda  
cool.
What i was saying was that when you store information concerning a  
playlist item, store ALL the information at the same location in the  
same way, so there is one clear way to acces, change and set that  
information. So here is what i think needs to be done in more detail.  
We should also look at the playlist systems of some other players and  
see if we can get any good ideas from them.

playlist_AddExt should be removed and playlist_AddItem() should be  
accompanied by a playlist_GetInfo() and playlist_AddInfo() [Which is  
also able to change].
There should be a special section called Options that holds the same  
information as the current options array. The special section "General"  
should be specified. What information is allowed in that section and  
what the format/unit's that it is in. This should be a standardized  
section.
URI, name/title, album/cd/vcd/dvd, author, duration spring to mind atm.  
For mp3 tags and cddb this information should also be completely stored  
in their own separate section. This way we separate 'general' info from  
a playlist_item, from complete id3 or cddb info.
I haven't quite fit DVD's into this, but that can be done as well.

The basic playlist_item structure should contain this playlist_info  
struct, the item's URI/MRL, the items's original index (might be useful  
when sorting etc. you never know), how many times the item has been  
played, the type (directory, playlist, url, audio file, video file, SAP  
[It think we should merge group and type....]), autodeletion, status,  
enabled. All strings stored in UTF-8.

The playlist should contain the playlist_items, the groups/types, the  
current index, size and status, the last sorting that was applied,
It should have a lot of accessor methods to reach and use all this  
information. We actually are pretty far with this already, it's just  
that some parts have been completely forgotten. We also need to split  
between playlist accessor methods and playlist_item accessor methods.

> Playlist items can span many physical media whereas the stream and
> "File" info must refer to the currently running one. (Instead of "File
> info", I prefer "Media Info"). And in the playlist window right now,
> there is no section for global or media-specific info. This could be
> rectified, but someone needs to do the work.

I prefer more general the term "Info". the most basic should go into a  
General section as discussed before.
We should actually allow the user to determine which columns to show.  
that's a lot more useful.
Simply because you can never contain complete MP3 tag info, CDDB info  
and URL info in the same playlist. They just differ too much. Allow the  
user to set what he likes to see there.

> I suggest that another place that one might consider showing global
> media information might be in the "Open target" window. In this window
> when you click on the "File" tab you see a list of directory and file
> names. Why not allow when you "browse" on a CD-DA or VCD, seeing a
> list of things (actually I was thinking MRLs) that constitute the CD.
> For the directory information one could give the media type (CD-DA,
> VCD, ISO-9660 filesystem) and some media information like title of CD.

Those routines still are extremely platform dependent. Perhaps one day  
with libcdio this will change, we should strive towards it, but it will  
take ages. And it has nothing to do with the playlist system.

> Or one could show the media information down on the bottom of the
> control panel where the MRL is shown.
I disagree. we should use the OSD for this. much more useful and less  
work.

> There are a number of possibilities, alas only the Media Info window
> of wxwindows works right now and without too much hassle.

> Information fields like the length of a CD or the CD Title has the
> scope of the entire CD; if these fields were copied into each playlist
> item there would be an update and duplication problem. So what you'd
> want here, as I think others have mentioned, would be a pointer to
> this common global information.
True, but isn't this information also duplicated in every MP3 for  
instance? I don't think this is truly a problem. total CD size in time  
is irrelevant to us, because you can just as easily fit a DVD in  
between your Audio CD tracks.
>
> There already is an infos ppsz array. I don't think it's is used by
> anything and therefore probably nobody is populating it either.
I think you are talking by the options array, which is used. it's used  
to bind options to a entry. like subtitle options, stream output  
options etc.

>> Then, the user should be able to see all the infos that are stored in
>> the pp_infos in the iteminfo dialog, and, perhaps, one day, be able to
>> show this in the listview (customizable, eh :).
>
> The part that gets me is the "one day." The input_AddInfo() and
> input_AddCategory routines were easy to use. Easier than the playlist
> stuff. So right now I have a way to show this information, even if it
> is not necessarily to everyone's liking or the most ideal situation.
>
> That's not to say things couldn't stand improvment or that I'm not for
> a *better* or more use-friendly ways of doing things.

I think we are all on the same line here by now.


---
Videolan - VLC media player
Derk-Jan Hartman (hartman at videolan dot org)
Developer of the MacOS X port of vlc
http://www.videolan.org/vlc

-- 
This is the vlc-devel mailing-list, see http://www.videolan.org/vlc/
To unsubscribe, please read http://developers.videolan.org/lists.html
If you are in trouble, please contact <postmaster at videolan.org>