[vlc-devel] [PATCH 1/2] lua/libs/input: fix 17611: properly populate filename attribute

[Filip Roséen]

Hi Pierre,

On 2016-11-06 22:11, Pierre Ynard wrote:

> > The previous implementations usage of input_item_GetName resulted in
> > unexpected data in the filename attribute
> 
> I can agree so far.
> 
> > (given that input_item_GetName will first query vlc_meta_Title, and if
> > present return that).
> 
> I don't follow, that would be input_item_GetTitleFbName() instead.

The usage of `vlc_meta_Title` is perhaps confusing (and a little
misleading since I should have mentined the name entity on its own
too), but I will try to explain the relevant code-paths.

`item->psz_name`, the value yield by `input_item_GetName` is the name
set for an item via either `input_item_SetName` or the relevant
initializer passed to `input_item_NewExt`.

`input_item_SetName` is however called from within
`src/input/es_out.c:EsOutMeta`, with the value of `vlc_meta_Title` (if
that is available); giving the semantics previously explained.

In other words; when you start playing an item, the *name* will update
to that of *Title* if such metadata is available.

> The only other relationship I'm aware of between name field and title
> metadata is that the lua binding to add new items copies from name to
> title if the title is missing.

See previous comment.

> > These changes extracts the filename for a given item, including
> > support for trailing slashes (so that we do not get an empty filename
> > for a path such as file:///media/).
> 
> That's a change in the API that can break current users. Have you
> analyzed the potential users relying on that behavior, in tree and out
> of tree? I see at least two meta scripts in tree (both seemingly broken
> for years).

Yes, I have analyzed the usage and both `02_frenchtv.lua` and
`filename.lua` actually expects the filename to be within the field.

Given that these two expects the filename to be there, as well as the
field-name suggesting that it is populated by the filename; I did not
want to remove the field (since, as you say, it is a breaking change).

I also took a look at public repositories at *github*, only finding
usages where the actual *filename* is expected (not a conditional
merge).
 
> I'm not sure what's your real problem here. I agree that this field is
> confusing, "filename" is misleading and it doesn't necessary belong
> within metadata. The name field is exposed in the CLI through the
> misnamed get_title command. The URI is exposed through the lua bindings
> through item:uri(), and the "file name" part of it can be easily
> obtained from it; alhough it's not available in the CLI.

Somewhat, yes; though it suffers from the same problem as fixed by
`24d0d1e`.

> I'm all for 1/ deprecating that field, 2/ improving known users of that
> weird field to use something else like item:uri() or item:name() and 3/
> improving CLI commands to display more, relevant information, but moving
> away from that metadata abuse. Aliasing get_title to get_name, adding a
> get_uri, and adding a section to info or a new command summarizing lua
> item fields, all sound like good ideas to me.

Agreed, though I was looking for an initial fix while we await a
bigger refactoration of the relevant paths.

 
> > +    if( psz_filename && psz_filename[1] == '\0' )
> > +    {
> > +        psz_filename[0] = '\0';
> > +        psz_filename = strrchr( psz_uri, '/' );
> > +    }
> 
> In any case this could use a comment because it took me way too long to
> understand what it's for.

I was actually about to introduce a helper for *filename*-extraction
given that we have several cases in the codebase where this is done,
but decided not to.

The lack of comment is probably due to me writing such code from the
back of my head. C, the language where you reinvent the wheel (or
bring an old one) in every new project. `;-)`
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161107/03a282d9/attachment.html>