[FFmpeg-devel] [RFC] Split libavformat

[Luca Abeni]

Hi Diego,

thanks for the review. I am quite busy at work right now, but tomorrow
I'll fix my patches according to it.

Diego Biurrun wrote:
[...]
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ ffmpeg/libavdevice/Makefile	2007-11-19 19:57:22.061554240 +0100
>> @@ -0,0 +1,36 @@
>> +#
>> +# libavdevice Makefile
>> +# (c) 2000-2003, 2007 Fabrice Bellard
>> +#
> 
> Fabrice did not write this.

Yes, I know... This is why I said that I had problems when writing the
license headers ;-)
I know that the current build system is mainly by you and Mans, but it
seems that you assigned the copyright to Fabrice... So I did the same.
Anyway, I'll fix this (in libavdevice and libavnet). What about something
like the following:
(c) 2000-2003 Fabrice Bellard
(c) 2000-2003, 2007 Diego Biurrun
(c) 2000-2003, 2007 Mans Rullgard

I do not know if removing Fabrice's copyright is legal, so I propose to
keep it.
I do not know the correct years for your and Mans's copyright... Let me
know the correct ones


[...]
>> +CFLAGS += -I$(SRC_PATH)/libavcodec -I$(SRC_PATH)/libavformat
> 
> Are both really needed?
It seems so: I include avformat.h, which in turns includes avcodec.h.
So, both the "-I" seem to be needed.


>> +# muxers/demuxers
>> +OBJS-$(CONFIG_BKTR_DEMUXER)              += bktr.o
> 
> Is there a better name than (de)muxers for this?

I do not know... Whatever people think it's better. Let me know, and
I'll update the makefile (ugh... Changing "CONFIG_*MUXER in something
else will require to change configure too... I am scared :)


>> +EXTRALIBS := -L$(BUILD_ROOT)/libavutil -lavutil$(BUILDSUF) \
>> +             -lavcodec$(BUILDSUF) -L$(BUILD_ROOT)/libavcodec \
>> +             -lavformat$(BUILDSUF) -L$(BUILD_ROOT)/libavformat $(EXTRALIBS)
> 
> Is all of this really needed?

Ops... No, I do not think it is needed... I blindly copied it from
somewhere else, without thinking. I'll remove this entry.


>> +NAME=avdevice
>> +LIBVERSION=$(LAVFVERSION)
>> +LIBMAJOR=$(LAVFMAJOR)
> 
> I think I prefer these lines at the top of the Makefile, but yes, this
> is inconsistently handled in the other Makefiles.

If you prefer, I can move them to the top


>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ ffmpeg/libavdevice/allformats.c	2007-11-19 19:57:22.062554088 +0100
>> @@ -0,0 +1,57 @@
>> +/*
>> + * Register all the grabbing formats 
>> + * Copyright (c) 2000, 2001, 2002, 2007 Fabrice Bellard
> 
> Fabrice did not write this.

Same as above: I copied from libavformat/allformats.c. I can add other
people's copyright if needed. Just let me know names and years.


>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ ffmpeg/libavdevice/avdevice.h	2007-11-19 19:57:22.063553936 +0100
>> @@ -0,0 +1,33 @@
>> +/*
>> + * copyright (c) 2001, 2007 Fabrice Bellard
> 
> ditto

I suspect I wrote it :). But I do not believe this file is copyrightable
at all... I just copied the header from libavformat (I would have no
problems assigning the copyright to Fabrice). Anyway, would it be ok to
just remove the copyright line? (I do not know if it would be legal, but
I am not a lawyer).


>> +#ifndef FFMPEG_AVDEV_H
>> +#define FFMPEG_AVDEV_H
>> +
>> +#endif /* FFMPEG_AVDEV_H */
> 
> Please use the filename as multiple inclusion guard.

Sorry, I forgot to update after a rename. I'll fix this.


>> --- ffmpeg.orig/configure	2007-11-19 19:49:07.299769464 +0100
>> +++ ffmpeg/configure	2007-11-19 19:57:22.066553480 +0100
>> @@ -920,6 +920,8 @@
>>  BSF_LIST=`sed -n 's/^[^#]*BSF.*(.*, *\(.*\)).*/\1_bsf/p' "$source_path/libavcodec/allcodecs.c"`
>>  MUXER_LIST=`sed -n 's/^[^#]*_MUX.*(.*, *\(.*\)).*/\1_muxer/p' "$source_path/libavformat/allformats.c"`
>>  DEMUXER_LIST=`sed -n 's/^[^#]*DEMUX.*(.*, *\(.*\)).*/\1_demuxer/p' "$source_path/libavformat/allformats.c"`
>> +MUXER_LIST="$MUXER_LIST `sed -n 's/^[^#]*_MUX.*(.*, *\(.*\)).*/\1_muxer/p' "$source_path/libavdevice/allformats.c"`"
>> +DEMUXER_LIST="$DEMUXER_LIST `sed -n 's/^[^#]*DEMUX.*(.*, *\(.*\)).*/\1_demuxer/p' "$source_path/libavdevice/allformats.c"`"
> 
> I'd rather see this as a new list.

Ok... I guess this will require more changes in the configure script.
I'll try to see what can I do.



				Thanks,
					Luca