[vlc-devel] [PATCH 01/20] input: item: add input_item_Parse
Thomas Guillem
thomas at gllm.fr
Mon Jun 3 07:53:08 CEST 2019
Fixed all nitpicks. Thanks.
On Sat, Jun 1, 2019, at 11:37, Alexandre Janniaux wrote:
> Hi,
>
> Nice, I have some minor nitpicking inline remarks.
>
> On Fri, May 31, 2019 at 03:59:27PM +0200, Thomas Guillem wrote:
> > This function doesn't use a background thread, contrary to
> > libvlc_MetadataRequest(). It will be used by the Medialibray that has its own
> > background thread.
> > ---
> > include/vlc_input_item.h | 60 +++++++++++++++++++++++++++++++++++
> > src/input/item.c | 67 ++++++++++++++++++++++++++++++++++++++++
> > src/libvlccore.sym | 2 ++
> > 3 files changed, 129 insertions(+)
> >
> > diff --git a/include/vlc_input_item.h b/include/vlc_input_item.h
> > index 5ae437c13f..272f774306 100644
> > --- a/include/vlc_input_item.h
> > +++ b/include/vlc_input_item.h
> > @@ -371,6 +371,66 @@ VLC_API input_item_t *input_item_Hold(input_item_t *);
> > /** Releases an input item, i.e. decrements its reference counter. */
> > VLC_API void input_item_Release(input_item_t *);
> >
> > +/**
> > + * input item parser opaque structure
> > + */
> > +typedef struct input_item_parser_t input_item_parser_t;
> > +
> > +/**
> > + * input item parser callbacks
> > + */
> > +typedef struct input_item_parser_cbs_t
> > +{
> > + /**
> > + * Event received when the parser ends
> > + *
> > + * @note this callback is mandatory.
>
> Nit, missing capital letter for "this".
>
> > + *
> > + * @param item the parsed item
> > + * @param ret VLC_SUCCESS in case of success, an error otherwise
>
> Would "status" or "parse_status" be a better name than ret ?
>
> > + * @param userdata user data set by input_item_Parse()
> > + */
> > + void (*on_ended)(input_item_t *item, int ret, void *userdata);
> > +
> > + /**
> > + * Event received when a new subtree is added
> > + *
> > + * @note this callback is optional.
>
> Nit, missing capital letter for "this".
>
> > + *
> > + * @param item the parsed item
> > + * @param subtree sub items of the current item
> > + * @param userdata user data set by input_item_Parse()
> > + */
> > + void (*on_subtree_added)(input_item_t *item, input_item_node_t *subtree, void *userdata);
> > +} input_item_parser_cbs_t;
> > +
> > +/**
> > + * Parse an item
>
> The brief text seems to mismatch the function, as it creates the parser
> but doesn't do the parsing. Maybe "parsing" could be explained too.
The returned parser is used as an opaque id in order to cancel it. Maybe input_item_parser_t is a bad name for it.
I don't see how the brief mismatches.
>
> > + *
> > + * @note the parsing is done asynchronously. The user can call
> > + * input_item_parser_Release() before receiving the on_ended() event in order
> > + * to interrupt it.
>
> Nit, missing capital letter for "the".
>
> > + *
> > + * @param item the item to parse
> > + * @param parent the parent obj
> > + * @param cbs callbacks to be notified of the end of the parsing
>
> It's not only the end of parsing but also subtree right ?
>
> > + * @param userdata opaque data user by parser callbacks
>
> I believed you meant s/user/used/.
>
> > + *
> > + * @return a parser instance or NULL in case of error, the parser needs to be
> > + * released with input_item_parser_Release()
> > + */
> > +VLC_API input_item_parser_t *
> > +input_item_Parse(input_item_t *item, vlc_object_t *parent,
> > + const input_item_parser_cbs_t *cbs, void *userdata) VLC_USED;
> > +
> > +/**
> > + * Release (and interrupt if needed) a parser
> > + *
> > + * @param parser the parser returned by input_item_Parse
> > + */
> > +VLC_API void
> > +input_item_parser_Release(input_item_parser_t *parser);
> > +
> > typedef enum input_item_meta_request_option_t
> > {
> > META_REQUEST_OPTION_NONE = 0x00,
> > diff --git a/src/input/item.c b/src/input/item.c
> > index 830c79895f..ce58b90122 100644
> > --- a/src/input/item.c
> > +++ b/src/input/item.c
> > @@ -1325,6 +1325,73 @@ void input_item_UpdateTracksInfo(input_item_t *item, const es_format_t *fmt)
> > vlc_mutex_unlock( &item->lock );
> > }
> >
> > +struct input_item_parser_t
> > +{
> > + input_thread_t *input;
> > + input_state_e state;
> > + const input_item_parser_cbs_t *cbs;
> > + void *userdata;
> > +};
> > +
> > +static void
> > +input_item_parser_InputEvent(input_thread_t *input,
> > + const struct vlc_input_event *event, void *parser_)
> > +{
> > + input_item_parser_t *parser = parser_;
> > +
> > + switch (event->type)
> > + {
> > + case INPUT_EVENT_STATE:
> > + parser->state = event->state;
> > + break;
> > + case INPUT_EVENT_DEAD:
> > + {
> > + int ret = parser->state == END_S ? VLC_SUCCESS : VLC_EGENERIC;
> > + parser->cbs->on_ended(input_GetItem(input), ret, parser->userdata);
> > + break;
> > + }
> > + case INPUT_EVENT_SUBITEMS:
> > + if (parser->cbs->on_subtree_added)
> > + parser->cbs->on_subtree_added(input_GetItem(input),
> > + event->subitems, parser->userdata);
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +input_item_parser_t *
> > +input_item_Parse(input_item_t *item, vlc_object_t *obj,
> > + const input_item_parser_cbs_t *cbs, void *userdata)
> > +{
> > + assert(cbs && cbs->on_ended);
> > + input_item_parser_t *parser = malloc(sizeof(*parser));
> > + if (!parser)
> > + return NULL;
> > +
> > + parser->state = INIT_S;
> > + parser->cbs = cbs;
> > + parser->userdata = userdata;
> > + parser->input = input_CreatePreparser(obj, input_item_parser_InputEvent,
> > + parser, item);
> > + if (!parser->input || input_Start(parser->input))
> > + {
> > + if (parser->input)
> > + input_Close(parser->input);
> > + free(parser);
> > + return NULL;
> > + }
> > + return parser;
> > +}
> > +
> > +void
> > +input_item_parser_Release(input_item_parser_t *parser)
> > +{
> > + input_Stop(parser->input);
> > + input_Close(parser->input);
> > + free(parser);
> > +}
> > +
> > static int rdh_compar_type(input_item_t *p1, input_item_t *p2)
> > {
> > if (p1->i_type != p2->i_type)
> > diff --git a/src/libvlccore.sym b/src/libvlccore.sym
> > index 87190a3573..ae7cc6d8b4 100644
> > --- a/src/libvlccore.sym
> > +++ b/src/libvlccore.sym
> > @@ -207,6 +207,8 @@ input_item_WriteMeta
> > input_item_slave_GetType
> > input_item_slave_New
> > input_item_AddSlave
> > +input_item_Parse
> > +input_item_parser_Release
> > input_Read
> > input_resource_New
> > input_resource_Release
> > --
> > 2.20.1
> >
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list