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

[Ronald S. Bultje]

Hi,

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:
>> Hi Peter,
>>
>> 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?

>> >> 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.
>>
>> > +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.

Ronald