On 01/06/12 01:58, Martin Storsjö wrote:
> On Thu, 31 May 2012, Luca Barbato wrote:
>
>> Adobe specifies onTextData as the standard message to use to deliver
>> text information.
>> ---
>> libavformat/flvdec.c | 107
>> +++++++++++++++++++++++++++++++++++++++++++-------
>> libavformat/flvenc.c | 73 +++++++++++++++++++++++++++++-----
>> 2 files changed, 155 insertions(+), 25 deletions(-)
>>
>> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
>> index b9d65a6..b6ad0a1 100644
>> --- a/libavformat/flvdec.c
>> +++ b/libavformat/flvdec.c
>> @@ -389,11 +403,11 @@ static int flv_read_header(AVFormatContext *s)
>> s->ctx_flags |= AVFMTCTX_NOHEADER;
>>
>> if(flags & FLV_HEADER_FLAG_HASVIDEO){
>> - if(!create_stream(s, 0))
>> + if(!create_stream(s, 0, AVMEDIA_TYPE_VIDEO))
>> return AVERROR(ENOMEM);
>> }
>> if(flags & FLV_HEADER_FLAG_HASAUDIO){
>> - if(!create_stream(s, 1))
>> + if(!create_stream(s, 1, AVMEDIA_TYPE_AUDIO))
>> return AVERROR(ENOMEM);
>> }
>>
>> @@ -453,6 +467,66 @@ static void clear_index_entries(AVFormatContext
>> *s, int64_t pos)
>> }
>> }
>>
>> +
>> +static int flv_data_packet(AVFormatContext *s, AVPacket *pkt,
>> + int64_t dts, int64_t next)
>> +{
>> + int ret = AVERROR_INVALIDDATA, i;
>> + AVIOContext *pb = s->pb;
>> + AVStream *st = NULL;
>> + AMFDataType type;
>> + char buf[20];
>> + int codec_id;
>> + int length;
>> +
>> + type = avio_r8(pb);
>> + if (type == AMF_DATA_TYPE_MIXEDARRAY)
>> + avio_seek(pb, 4, SEEK_CUR);
>> + else if (type != AMF_DATA_TYPE_OBJECT)
>> + goto out;
>> +
>> + amf_get_string(pb, buf, sizeof(buf));
>> + if (strcmp(buf, "type") || avio_r8(pb) != AMF_DATA_TYPE_STRING)
>> + goto out;
>> +
>> + amf_get_string(pb, buf, sizeof(buf));
>> + //FIXME parse it as codec_id
>> + amf_get_string(pb, buf, sizeof(buf));
>> + if (strcmp(buf, "text") || avio_r8(pb) != AMF_DATA_TYPE_STRING)
>> + goto out;
>> +
>> + length = avio_rb16(pb);
>> + ret = av_get_packet(s->pb, pkt, length);
>> + if (ret < 0) {
>> + ret = AVERROR(EIO);
>> + goto out;
>> + }
>> +
>> + for (i=0; i<s->nb_streams; i++) {
>> + st = s->streams[i];
>> + if (st->id == 2)
>> + break;
>> + }
>> +
>> + if (st->id != 2) {
>> + st = create_stream(s, 2, AVMEDIA_TYPE_DATA);
>
> This logic looks weird. If there are no streams at all (not sure if
> that's possible), it would segfault. If you change this check to if (i
> == s->nb_streams), this would be less fragile.
>
> Anton also had some concerns about the stream/id handling in general in
> the last review round of the patch - did you comment on that somewhere?
Either I scratch this patchset and first change the flvdec not to use id
that way at all and then add the feature or I do that in the other way
round. The patch is consistent with the current code.
>> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
>> index 3a51aaa..c07cad2 100644
>> --- a/libavformat/flvenc.c
>> +++ b/libavformat/flvenc.c
>> @@ -187,7 +187,8 @@ static int flv_write_header(AVFormatContext *s)
>> for(i=0; i<s->nb_streams; i++){
>> AVCodecContext *enc = s->streams[i]->codec;
>> FLVStreamContext *sc;
>> - if (enc->codec_type == AVMEDIA_TYPE_VIDEO) {
>> + switch (enc->codec_type) {
>> + case AVMEDIA_TYPE_VIDEO:
>> if (s->streams[i]->r_frame_rate.den &&
>> s->streams[i]->r_frame_rate.num) {
>> framerate = av_q2d(s->streams[i]->r_frame_rate);
>> } else {
>> @@ -198,10 +199,22 @@ static int flv_write_header(AVFormatContext *s)
>> av_log(s, AV_LOG_ERROR, "video codec not compatible
>> with flv\n");
>> return -1;
>> }
>> - } else {
>> + break;
>> + case AVMEDIA_TYPE_AUDIO:
>> audio_enc = enc;
>> if (get_audio_flags(s, enc) < 0)
>> - return -1;
>> + return AVERROR_INVALIDDATA;
>> + break;
>> + case AVMEDIA_TYPE_DATA:
>> + if (enc->codec_id != CODEC_ID_TEXT) {
>> + av_log(s, AV_LOG_ERROR, "codec not compatible with
>> flv\n");
>> + return AVERROR_INVALIDDATA;
>> + }
>> + data_enc = enc;
>> + break;
>> + default:
>> + av_log(s, AV_LOG_ERROR, "codec not compatible with flv\n");
>> + return -1;
>
> The placing of the breaks are weird here - I see that you fix up that in
> the third patch, but please squash the hunks that touch the newly added
> code into this one.
I'd rather keep the thing consistent. I guess I'd redo the patchset
first going over the codebase so it is cleaner, then adding this feature
and a couple others, in the end the onTextData is a couple of functions.
lu
--
Luca Barbato
Gentoo/linux
http://dev.gentoo.org/~lu_zero
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
