[Ffmpeg-devel] [PATCH] FLV metadata decoding

[Allan Hsu]

On Nov 30, 2006, at 1:23 AM, Michael Niedermayer wrote:

> Hi

[...]

> tabs arent allowed in svn

Is there a standard for how many spaces are used for each level of  
indentation in ffmpeg code? I apologize for not noticing the use of  
spaces and not tabs in the existing code. I tried to find ffmpeg  
coding guidelines on the web site, but I could not.

> this should be an enum

[...]

> all the defines should be in a common header like flv.h and they  
> should
> be used by the muxer and demuxer, and this should be a seperate patch

For clarification, would it be ok to write a patch that puts the  
defines and enums into a new common header and changes the hardcoded  
values in flvenc.c to the new defines in the common header, then  
write a second patch that revisits the metadata reading code in  
flvdec.c?

>
>> +
>> +typedef struct {
>> +	double duration;
>> +	int videocodecid;
>> +	int audiocodecid;
>> +	double videoframerate;
>> +	unsigned int videowidth;
>> +	unsigned int videoheight;
>> +	unsigned int audiosamplerate;
>> +	unsigned int audiosamplesize;
>> +	unsigned int audiochannels;
>> +} flv_header_info;
>
> information should be directly read into AVStream/AVCodecContext, an
> additional layer is not ok

I read the data into a temporary place because I didn't want to  
create a stream unless a complete set of information was available  
from the metadata tag. The data in the onMetaData tag isn't  
guaranteed to contain enough information to properly initialize the  
streams. Is there a better convention for doing this?

>
>> +
>> +static int flv_meta_read_metadata(AVFormatContext *s, int flags);
>> +static int flv_meta_read_mixedarray(AVFormatContext *s,  
>> flv_header_info *header_info, unsigned int pos_limit);
>> +static int flv_data_get_string(ByteIOContext *ioc, char *buffer,  
>> int maxlen);
>> +static int flv_data_skip_object(ByteIOContext *ioc, unsigned int  
>> *type);
>
> useless

Do you mean that the function prototypes are useless, or the  
functions themselves?

>
>> +
>> +static int flv_meta_read_metadata(AVFormatContext *s, int flags) {
>> +	flv_header_info header_info;
>> +	ByteIOContext *ioc = &s->pb;
>> +	offset_t offset;
>> +	unsigned int type, bodylength, next_tag;
>> +	char buffer[11];
>
> the metadata parsing is duplicated, ANY code duplication causes a  
> patch
> to be rejected

Would it be acceptable to abstract the metadata parsing into a  
function shared by both the header reading code and the packet  
reading code?

>
> [...]	
>> +		if(header_info.audiocodecid == 0 || header_info.audiochannels  
>> == 0 || header_info.audiosamplerate == 0
>> +		   || header_info.audiosamplesize == 0)
>> +			goto bail;
>
> this stuff is unrelated to AVFMTCTX_NOHEADER IIRC,  
> AVFMTCTX_NOHEADER is
> just about the possibility of adding new streams in random packets
>
> basically a 2 line change like
> if(nb_streams==2)
>     flags &= ~AVFMTCTX_NOHEADER;
>
> in read_packet() should fix the issue if there is an audio and  
> video stream
> if there is not then i dont see how parsing a large chunk of the  
> file can
> be avoded, nothing gurantees that no video or audio stream starts  
> at a later
> point or is there some rule in the spec or some no-audio-stream  
> metadata i missed?
>
> but besides this, metadata reading support is certainly welcome ...

The fifth byte of every FLV file contains a bitmask that signals the  
existence of audio or video streams. Some FLV metadata tools will  
also report this in the onMetaData tag, but I think it's for the  
benefit of Flash code written in actionscript that doesn't  
necessarily have easy access to the raw data stream. Regardless of  
the information in the onMetaData tag, we should always know whether  
or not either stream exists.

[...]

I will attempt to rework this patch and resubmit it.

	-Allan

--
Allan Hsu <allan at counterpop dot net>
1E64 E20F 34D9 CBA7 1300  1457 AC37 CBBB 0E92 C779