[vlc-devel] [PATCH 1/2] demux/subtitles: add new implementation for line-peeking
Rémi Denis-Courmont
remi at remlab.net
Mon Nov 7 18:08:52 CET 2016
Le sunnuntaina 6. marraskuuta 2016, 20.34.24 EET Filip Roséen a écrit :
> Hi Remi,
>
> On 2016-11-06 21:29, Rémi Denis-Courmont wrote:
> > Le sunnuntaina 6. marraskuuta 2016, 17.58.44 EET Filip Roséen a écrit :
> > > This added function properly handles streams where an initial BOM is
> > > used to state the encoding used, effectively making sure that we do
> > > not forget about the encoding of the stream while peeking lines.
> > > ---
> > >
> > > modules/demux/subtitle_helper.h | 75
> > >
> > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75
> > > insertions(+)
> > >
> > > diff --git a/modules/demux/subtitle_helper.h
> > > b/modules/demux/subtitle_helper.h index b5f18da..fc71c12 100644
> > > --- a/modules/demux/subtitle_helper.h
> > > +++ b/modules/demux/subtitle_helper.h
> > > @@ -18,6 +18,9 @@
> > >
> > > * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> > >
> > > ************************************************************************
> > > ***
> > > **/
> > >
> > > +#include <vlc_common.h>
> > > +#include <vlc_stream.h>
> > > +
> > >
> > > inline static char * peek_Readline( stream_t *p_demuxstream, uint64_t
> > >
> > > *pi_offset ) {
> > >
> > > uint8_t *p_peek;
> > >
> > > @@ -43,3 +46,75 @@ inline static char * peek_Readline( stream_t
> > > *p_demuxstream, uint64_t *pi_offset
> > >
> > > return psz_line;
> > >
> > > }
> > >
> > > +
> > > +/**
> > > + * Peek next line of stream
> > > + *
> > > + * This function returns the next available line in \p p_source
> > > + * without modifying the read-offset of the stream (meaning that it
> > > + * effectivelly it behaves like vlc_stream_Peek, but for lines).
> > > + *
> > > + * \param source The stream for which line-peeking shall be made
> > > + * \param peekstate shall be NULL on first invocation, and `*peekstate`
> > > + * will be updated to reflect the internal state of
> > > the
> > > + * peeking.
> > > + *
> > > + * \warning if `*peekstate` is not NULL, it shall be released by the
> > > + * callee when no more data is desired to be peeked (using
> > > + * \link vlc_stream_Delete).
> > > + *
> > > + * \return pointer to the next line (if any), must be freed by the
> > > + * callee.
> > > + **/
> > > +inline static char * subtitle_PeekLine( stream_t *source, stream_t
> > > **peekstate ) +{
> > > + for( uint64_t offset = 0, size = 0;; )
> > > + {
> > > + char* line = NULL;
> > > +
> > > + if( *peekstate )
> > > + {
> > > + if( vlc_stream_GetSize( *peekstate, &size ) )
> > > + return NULL;
> >
> > The whole point of the faulty commit was to support probing subtitles from
> > nonseekable streams. Nonseekable streams typically do not have a known
> > size, so this is an extremely dubious requirement.
> >
> > In other words, reverting the faulty commit would be a lot easier.
>
> `*peekstate` refers to a `stream_t` created by `vlc_stream_MemoryNew`,
> as such it is safe to do the operations within `subtitle_PeekLine` (it
> is not related to seekability of the demuxer stream).
Well OK, that invalidates my comments. But it still does not seem right.
At best, you end up with quadratic complexity processing the same lines over
and over again. But IMHO the worst lies in second guessing
vlc_stream_ReadLine().
If you want to peek lines, you should rather use a proper stream wrapper than
create memory streams over and over again, e.g. (completely untested):
diff --git a/src/input/stream_peek.c b/src/input/stream_peek.c
new file mode 100644
index 0000000..797b0f4
--- /dev/null
+++ b/src/input/stream_peek.c
@@ -0,0 +1,98 @@
+/*****************************************************************************
+ * stream_peek.c
+ *****************************************************************************
+ * Copyright (C) 2016 Rémi Denis-Courmont
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; either version 2.1 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
+ *****************************************************************************/
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <limits.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <vlc_common.h>
+#include <vlc_stream.h>
+
+#include "stream.h"
+
+static void vlc_stream_peek_Destroy(stream_t *s)
+{
+ (void) s;
+}
+
+static ssize_t vlc_stream_peek_Read(stream_t *s, void *buf, size_t len)
+{
+ stream_t *parent = (stream_t *)s->obj.parent;
+ const uint8_t *peek;
+ size_t offset = (uintptr_t)s->p_sys;
+
+ if (unlikely(len > SSIZE_MAX - offset))
+ len = SSIZE_MAX - offset;
+
+ ssize_t ret = vlc_stream_Peek(parent, &peek, offset + len);
+ if (ret <= (ssize_t)offset)
+ return 0;
+
+ s->p_sys = (void *)(uintptr_t)ret;
+ len = ret - offset;
+
+ if (buf != NULL)
+ memcpy(buf, peek + offset, len);
+
+ return len;
+}
+
+static int vlc_stream_peek_Control(stream_t *s, int query, va_list ap)
+{
+ stream_t *parent = (stream_t *)s->obj.parent;
+
+ switch (query)
+ {
+ case STREAM_CAN_SEEK:
+ case STREAM_CAN_FASTSEEK:
+ case STREAM_CAN_PAUSE:
+ case STREAM_CAN_CONTROL_PACE:
+ case STREAM_GET_SIZE:
+ case STREAM_IS_DIRECTORY:
+ case STREAM_GET_PTS_DELAY:
+ case STREAM_GET_TITLE_INFO:
+ case STREAM_GET_TITLE:
+ case STREAM_GET_SEEKPOINT:
+ case STREAM_GET_META:
+ case STREAM_GET_CONTENT_TYPE:
+ case STREAM_GET_SIGNAL:
+ return vlc_stream_vaControl(parent, query, ap);
+ default:
+ return VLC_EGENERIC;
+ }
+}
+
+stream_t *vlc_stream_peek_New(stream_t *parent)
+{
+ stream_t *s = vlc_stream_CommonNew(VLC_OBJECT(parent),
+ vlc_stream_peek_Destroy);
+ if (unlikely(s == NULL))
+ return NULL;
+
+ s->pf_read = vlc_stream_peek_Read;
+ s->pf_seek = NULL;
+ s->pf_control = vlc_stream_peek_Control;
+ s->p_sys = (void *)(uintptr_t)0;
+ return s;
+}
>
> > > +
> > > + offset = vlc_stream_Tell( *peekstate );
> > > + line = vlc_stream_ReadLine( *peekstate );
> >
> > Likewise, you can´t do that here; this is missing the point.
>
> See previous comment.
>
> > > +
> > > + if( line && vlc_stream_Tell( *peekstate ) < size )
> > > + return line;
> > > + }
> > > +
> > > + uint8_t const* data;
> > > + ssize_t peeked = vlc_stream_Peek( source, &data, size + 2048 );
> > > +
> > > + if( peeked > 0 && (uint64_t)peeked <= size )
> > > + return line;
> > > + else
> > > + free( line );
> > > +
> > > + if( peeked < 0 )
> > > + return NULL;
> > > +
> > > + if( *peekstate )
> > > + {
> > > + vlc_stream_Delete( *peekstate );
> > > + *peekstate = NULL;
> > > + }
> > > +
> > > + *peekstate = vlc_stream_MemoryNew( source, (uint8_t*)data,
> > > + peeked, true );
> > > + if( unlikely( !*peekstate ) )
> > > + return NULL;
> > > +
> > > + if( offset != 0 )
> > > + {
> > > + /* read potential BOM */
> > > + free( vlc_stream_ReadLine( *peekstate ) );
> > > +
> > > + if( vlc_stream_Seek( *peekstate, offset ) )
> > > + return NULL;
> > > + }
> > > + }
> > > +
> > > + vlc_assert_unreachable();
> > > +}
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list