On 01/06/12 08:43, Luca Barbato wrote:
> 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.
The patch is still large enough to make it boring to redo that again for
the third time.
--
Luca Barbato
Gentoo/linux
http://dev.gentoo.org/~lu_zero
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
