[FFmpeg-devel] [RFC][PATCH] Windows Television (WTV) file system handling

Sun Jan 23 02:43:03 CET 2011

On Sat, Jan 22, 2011 at 11:37:28AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Sat, Jan 22, 2011 at 5:20 AM, Peter Ross <pross at xvid.org> wrote:
> > On Thu, Jan 20, 2011 at 06:39:11PM -0500, Ronald S. Bultje wrote:
> >> On Thu, Jan 20, 2011 at 6:31 PM, Peter Ross <pross at xvid.org> wrote:
> >> > On Thu, Jan 20, 2011 at 09:03:57AM -0500, Ronald S. Bultje wrote:
> >> >> On Thu, Jan 20, 2011 at 12:19 AM, Peter Ross <pross at xvid.org> wrote:
> >> >> > On Sun, Jan 09, 2011 at 05:21:20PM +1100, Peter Ross wrote:
> >> >> >> 0002-add-AVFMT_NOGENERICSEEK-flag.patch
> >> >> >> * adds AVFMT_NOGENERICSEEK flag, which should be self explanatory.
> >> >> [..]
> >> >> > @@ -23,7 +23,7 @@
> >> >> >
> >> >> > ?#define LIBAVFORMAT_VERSION_MAJOR 52
> >> >> > ?#define LIBAVFORMAT_VERSION_MINOR 92
> >> >> > -#define LIBAVFORMAT_VERSION_MICRO ?0
> >> >> > +#define LIBAVFORMAT_VERSION_MICRO ?1
> >> >> >
> >> >> > ?#define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
> >> >> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? LIBAVFORMAT_VERSION_MINOR, \
> >> >> > @@ -324,6 +324,7 @@ typedef struct AVFormatParameters {
> >> >> > ?#define AVFMT_VARIABLE_FPS ?0x0400 /**< Format allows variable fps. */
> >> >> > ?#define AVFMT_NODIMENSIONS ?0x0800 /**< Format does not need width/height */
> >> >> > ?#define AVFMT_NOSTREAMS ? ? 0x1000 /**< Format does not require any streams */
> >> >> > +#define AVFMT_NOGENERICSEEK 0x2000 /**< Do not perform generic read_seek */
> >> >> >
> >> >> > ?typedef struct AVOutputFormat {
> >> >> > ? ? ?const char *name;
> >> >> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> >> >> > index 32067e9..365272b 100644
> >> >> > --- a/libavformat/utils.c
> >> >> > +++ b/libavformat/utils.c
> >> >> > @@ -1747,6 +1747,9 @@ int av_seek_frame(AVFormatContext *s, int stream_index, int64_t timestamp, int f
> >> >> > ? ? ? ? ?return 0;
> >> >> > ? ? ?}
> >> >> >
> >> >> > + ? ?if ((s->iformat->flags & AVFMT_NOGENERICSEEK))
> >> >> > + ? ? ? ?return -1;
> >> >> > +
> >> >> > ? ? ?if(s->iformat->read_timestamp)
> >> >> > ? ? ? ? ?return av_seek_frame_binary(s, stream_index, timestamp, flags);
> >> >> > ? ? ?else
> >> >>
> >> >> What does this do/fix?
> >> >
> >> > The demuxer sets AVIndexEntry->pos to positions relative to the start of a file
> >> > *within* the filesystem. If the demuxer's read_seek() fails, lavf trys to
> >> > perform a generic seek using the AVIndexEntry[]->pos values.
> >>
> >> I don't really have a strong opinion on this, but I would say that
> >> _maybe_ this is abuse of AVIndexEntry[]->pos in a way that it was not
> >> designed for. What do others think?
> >
> > Fair enough. The enclosed patch adds ff_add_index_entry() and ff_index_search()
> > for handling privately maintained indexes.
> >
> >> >> >> 0001-make-unicode-string-reader-accessible-to-other-modu.patch
> >> >> >> * the metadata format uses unicode structures similar, but not identical,
> >> >> >> ? to ASF/DRV-MS.
> >> >> [..]
> >> >> > ?/**
> >> >> > + * Read a null terminated string
> >> >> > + */
> >> >> > +static void get_utf16z(ByteIOContext *pb, char *buf, int buf_size)
> >> >> > +{
> >> >> > + ? ?char* q = buf;
> >> >> > + ? ?int ch;
> >> >> > + ? ?while ((ch = get_le16(pb)))
> >> >> > + ? ? ? ?if (q - buf < buf_size - 1) *q++ = ch;
> >> >> > + ? ?*q = '\0';
> >> >> > +}
> >> >>
> >> >> Already exists in asfdec.c, probably, and likely mms uses that one
> >> >> also. Can you reuse that function? It looks ok in general, I think
> >> >> we'd want some files in this format in fate so this sort of stuff is
> >> >> tested.
> >
> > There is ff_asf_get_str16_nolen() in asfdec, but it expects the caller to
> > know the string length in advance. We dont have that information in wtv.
> > get_utf16z() reads until it hits the null terminator.
> 
> Shit, ok, that kind of sucks. OK, if they're different, then they're different.
> 
> >> >> > +static const ff_asf_guid dir_entry_guid =
> >> >> > + ? ?{0x92,0xb7,0x74,0x91,0x59,0x70,0x70,0x44,0x88,0xdf,0x06,0x3b,0x82,0xcc,0x21,0x3d};
> >> >>
> >> >> Should it be in the asf header? Seems duplicated.
> >> >
> >> > Which part is duplicated?
> >>
> >> You're right, I was asleep. This part is fine.
> >>
> >> >> > +/**
> >> >> > + * @param[in] buf unicode buffer
> >> >> > + * @param buf_size buffer size (bytes)
> >> >> > + * @return 0 if equal
> >> >> > + */
> >> >> > +static int unicode_compare(const uint8_t *buf, int buf_size, const uint8_t *s)
> >> >>
> >> >> I'd be shocked if libc didn't provide some way of doing this, convert
> >> >> utf16->utf8 and then strcmp()? Also you don't check buf[i*2+1] ever, I
> >> >
> >> > I do not know of any. While there are wchar functions, one cannot rely on
> >> > sizeof(wchar_t)==2.
> >>
> >> If you insist on comparing strings, all strings being compared here
> >> are always static strings. This allows for tricks, e.g. declaring them
> >> as wchar_t strings, which you apparently don't want to do, or just
> >> creating one manually by doing "s\0t\0r\0i\0n\0g"; </ugly>. The
> >> advantage of these approaches (easiest is really to look for a
> >> wchar-style thing that ensures 16-bit unicode formatting) is that you
> >> can use memcmp() instead of this slow compare function, and memcmp()
> >> is probably better-optimized. Also saves code.
> >
> > You're right, that looks horrid. I have updated to use wcscmp().
> 
> That's not what I meant. You read a wtv string (in UCS2/UTF16LE/...
> format) and compare it against a static string that you define as a
> "string"; in wtv.c.
> a) you implemented unicode_compare to compare utf8 against it
> b) you convert the wtv string to wchar and compare it
> 
> what I want you to do is compare it _without_ converting anything.
> Since you control the format of the "string" in wtv.c, I want you to
> write it out in whatever the format is in the wtv file, and compare it
> using memcmp():
> 
> +    wtv->pb = wtvfile_open(s, root, root_size, L"timeline");
> [..]
> +    pb = wtvfile_open(s, root, root_size, L"table.0.entries.legacy_attrib");
> [..]
> etc
> 
> L is a wchar string, so what I want you to do is to convert it to a
> UTF16 string or whatever it is in wtv.c. Ugly way:
> "t\0i\0m\0e\0l\0i\0n\0e\0";, but there's probably less ugly ways or
> macros that you can use in some of libc. You can also use (uint16_t
> str[]){'t','i','m','e','l','i','n','e','\0'} although that has
> endianness issues so you need to probably make a macro
> UTF16LE_STR("bla") which does the magic for you in whatever way. I'm
> not very good at macros with loops inside them, but in the end what I
> want is a compare without a convert. In the worst case, like I said,
> just have some static uint8_t timeline_u16le[] = 't',0,'i',0,[etc.] in
> the file to help you get this done.

This looks good. Patch updated.

Have also fixed get_utf16lez to convert utf-16 -> utf-8.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-wtv-filesystem-implementation.patch
Type: text/x-diff
Size: 27169 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110123/da0e301f/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110123/da0e301f/attachment.pgp>