[libav-devel] [PATCH 1/2] rtmp: check ff_rtmp_packet_create calls.

Samuel Pitoiset samuel.pitoiset at gmail.com
Wed May 23 22:17:13 CEST 2012


On Tue, May 22, 2012 at 10:45 PM, Martin Storsjö <martin at martin.st> wrote:

> On Tue, 22 May 2012, Samuel Pitoiset wrote:
>
>  ---
>> libavformat/rtmppkt.c   |    6 +-
>> libavformat/rtmpproto.c |  223 ++++++++++++++++++++++++++++++**
>> +++++------------
>> 2 files changed, 173 insertions(+), 56 deletions(-)
>>
>> diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
>> index 8c455a0..00507d2 100644
>> --- a/libavformat/rtmppkt.c
>> +++ b/libavformat/rtmppkt.c
>> @@ -79,6 +79,7 @@ int ff_rtmp_packet_read(URLContext *h, RTMPPacket *p,
>>    uint32_t extra = 0;
>>    enum RTMPPacketType type;
>>    int size = 0;
>> +    int ret;
>>
>>    if (ffurl_read(h, &hdr, 1) != 1)
>>        return AVERROR(EIO);
>> @@ -129,8 +130,9 @@ int ff_rtmp_packet_read(URLContext *h, RTMPPacket *p,
>>    if (hdr != RTMP_PS_TWELVEBYTES)
>>        timestamp += prev_pkt[channel_id].**timestamp;
>>
>> -    if (ff_rtmp_packet_create(p, channel_id, type, timestamp, data_size))
>> -        return -1;
>> +    ret = ff_rtmp_packet_create(p, channel_id, type, timestamp,
>> data_size);
>> +    if (ret < 0)
>> +        return ret;
>>    p->extra = extra;
>>
>
> As Diego pointed out, this isn't what the commit message says. Include
> "pass the returned codes through" or something similar and it should be
> fine.
>
>
>     // save history
>>    prev_pkt[channel_id].channel_**id = channel_id;
>> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
>> index 1b86c44..4c46a1d 100644
>> --- a/libavformat/rtmpproto.c
>> +++ b/libavformat/rtmpproto.c
>> @@ -115,12 +115,17 @@ static const uint8_t rtmp_server_key[] = {
>> /**
>>  * Generate 'connect' call and send it to the server.
>>  */
>> -static void gen_connect(URLContext *s, RTMPContext *rt)
>> +static int gen_connect(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>> +
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 4096);
>> +    if (ret < 0)
>> +        return ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL, RTMP_PT_INVOKE, 0,
>> 4096);
>>    p = pkt.data;
>>
>>    ff_amf_write_string(&p, "connect");
>> @@ -165,19 +170,24 @@ static void gen_connect(URLContext *s, RTMPContext
>> *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate 'releaseStream' call and send it to the server. It should make
>>  * the server release some channel for media streams.
>>  */
>> -static void gen_release_stream(URLContext *s, RTMPContext *rt)
>> +static int gen_release_stream(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL, RTMP_PT_INVOKE, 0,
>> -                          29 + strlen(rt->playpath));
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 29 + strlen(rt->playpath));
>> +    if (ret < 0)
>> +        return ret;
>>
>>    av_log(s, AV_LOG_DEBUG, "Releasing stream...\n");
>>    p = pkt.data;
>> @@ -188,19 +198,24 @@ static void gen_release_stream(URLContext *s,
>> RTMPContext *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate 'FCPublish' call and send it to the server. It should make
>>  * the server preapare for receiving media streams.
>>  */
>> -static void gen_fcpublish_stream(**URLContext *s, RTMPContext *rt)
>> +static int gen_fcpublish_stream(**URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL, RTMP_PT_INVOKE, 0,
>> -                          25 + strlen(rt->playpath));
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 25 + strlen(rt->playpath));
>> +    if (ret < 0)
>> +        return ret;
>>
>>    av_log(s, AV_LOG_DEBUG, "FCPublish stream...\n");
>>    p = pkt.data;
>> @@ -211,19 +226,24 @@ static void gen_fcpublish_stream(**URLContext *s,
>> RTMPContext *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate 'FCUnpublish' call and send it to the server. It should make
>>  * the server destroy stream.
>>  */
>> -static void gen_fcunpublish_stream(**URLContext *s, RTMPContext *rt)
>> +static int gen_fcunpublish_stream(**URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL, RTMP_PT_INVOKE, 0,
>> -                          27 + strlen(rt->playpath));
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 27 + strlen(rt->playpath));
>> +    if (ret < 0)
>> +        return ret;
>>
>>    av_log(s, AV_LOG_DEBUG, "UnPublishing stream...\n");
>>    p = pkt.data;
>> @@ -234,19 +254,25 @@ static void gen_fcunpublish_stream(**URLContext
>> *s, RTMPContext *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate 'createStream' call and send it to the server. It should make
>>  * the server allocate some channel for media streams.
>>  */
>> -static void gen_create_stream(URLContext *s, RTMPContext *rt)
>> +static int gen_create_stream(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>>    av_log(s, AV_LOG_DEBUG, "Creating stream...\n");
>> -    ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL, RTMP_PT_INVOKE, 0,
>> 25);
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 25);
>> +    if (ret < 0)
>> +        return ret;
>>
>>    p = pkt.data;
>>    ff_amf_write_string(&p, "createStream");
>> @@ -256,6 +282,8 @@ static void gen_create_stream(URLContext *s,
>> RTMPContext *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>>
>> @@ -263,13 +291,17 @@ static void gen_create_stream(URLContext *s,
>> RTMPContext *rt)
>>  * Generate 'deleteStream' call and send it to the server. It should make
>>  * the server remove some channel for media streams.
>>  */
>> -static void gen_delete_stream(URLContext *s, RTMPContext *rt)
>> +static int gen_delete_stream(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>>    av_log(s, AV_LOG_DEBUG, "Deleting stream...\n");
>> -    ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL, RTMP_PT_INVOKE, 0,
>> 34);
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 34);
>> +    if (ret < 0)
>> +        return ret;
>>
>>    p = pkt.data;
>>    ff_amf_write_string(&p, "deleteStream");
>> @@ -279,20 +311,26 @@ static void gen_delete_stream(URLContext *s,
>> RTMPContext *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate 'play' call and send it to the server, then ping the server
>>  * to start actual playing.
>>  */
>> -static void gen_play(URLContext *s, RTMPContext *rt)
>> +static int gen_play(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>>    av_log(s, AV_LOG_DEBUG, "Sending play command for '%s'\n",
>> rt->playpath);
>> -    ff_rtmp_packet_create(&pkt, RTMP_VIDEO_CHANNEL, RTMP_PT_INVOKE, 0,
>> -                          29 + strlen(rt->playpath));
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_VIDEO_CHANNEL, RTMP_PT_INVOKE,
>> +                                0, 29 + strlen(rt->playpath));
>> +    if (ret < 0)
>> +        return ret;
>> +
>>    pkt.extra = rt->main_channel_id;
>>
>>    p = pkt.data;
>> @@ -306,7 +344,10 @@ static void gen_play(URLContext *s, RTMPContext *rt)
>>    ff_rtmp_packet_destroy(&pkt);
>>
>>    // set client buffer time disguised in ping packet
>> -    ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL, RTMP_PT_PING, 1,
>> 10);
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL, RTMP_PT_PING,
>> +                                1, 10);
>> +    if (ret < 0)
>> +        return ret;
>>
>>    p = pkt.data;
>>    bytestream_put_be16(&p, 3);
>> @@ -315,19 +356,25 @@ static void gen_play(URLContext *s, RTMPContext *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate 'publish' call and send it to the server.
>>  */
>> -static void gen_publish(URLContext *s, RTMPContext *rt)
>> +static int gen_publish(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>>    av_log(s, AV_LOG_DEBUG, "Sending publish command for '%s'\n",
>> rt->playpath);
>> -    ff_rtmp_packet_create(&pkt, RTMP_SOURCE_CHANNEL, RTMP_PT_INVOKE, 0,
>> -                          30 + strlen(rt->playpath));
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SOURCE_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 30 + strlen(rt->playpath));
>> +    if (ret < 0)
>> +        return ret;
>> +
>>    pkt.extra = rt->main_channel_id;
>>
>>    p = pkt.data;
>> @@ -339,48 +386,68 @@ static void gen_publish(URLContext *s, RTMPContext
>> *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return ret;
>> }
>>
>> /**
>>  * Generate ping reply and send it to the server.
>>  */
>> -static void gen_pong(URLContext *s, RTMPContext *rt, RTMPPacket *ppkt)
>> +static int gen_pong(URLContext *s, RTMPContext *rt, RTMPPacket *ppkt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>> +
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL, RTMP_PT_PING,
>> +                                ppkt->timestamp + 1, 6);
>> +    if (ret < 0)
>> +        return ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL, RTMP_PT_PING,
>> ppkt->timestamp + 1, 6);
>>    p = pkt.data;
>>    bytestream_put_be16(&p, 7);
>>    bytestream_put_be32(&p, AV_RB32(ppkt->data+2));
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate server bandwidth message and send it to the server.
>>  */
>> -static void gen_server_bw(URLContext *s, RTMPContext *rt)
>> +static int gen_server_bw(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>> +
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL,
>> RTMP_PT_SERVER_BW,
>> +                                0, 4);
>> +    if (ret < 0)
>> +        return ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL, RTMP_PT_SERVER_BW,
>> 0, 4);
>>    p = pkt.data;
>>    bytestream_put_be32(&p, 2500000);
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> /**
>>  * Generate check bandwidth message and send it to the server.
>>  */
>> -static void gen_check_bw(URLContext *s, RTMPContext *rt)
>> +static int gen_check_bw(URLContext *s, RTMPContext *rt)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL, RTMP_PT_INVOKE, 0,
>> 21);
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_SYSTEM_CHANNEL,
>> RTMP_PT_INVOKE,
>> +                                0, 21);
>> +    if (ret < 0)
>> +        return ret;
>>
>>    p = pkt.data;
>>    ff_amf_write_string(&p, "_checkbw");
>> @@ -389,21 +456,30 @@ static void gen_check_bw(URLContext *s, RTMPContext
>> *rt)
>>
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return ret;
>> }
>>
>> /**
>>  * Generate report on bytes read so far and send it to the server.
>>  */
>> -static void gen_bytes_read(URLContext *s, RTMPContext *rt, uint32_t ts)
>> +static int gen_bytes_read(URLContext *s, RTMPContext *rt, uint32_t ts)
>> {
>>    RTMPPacket pkt;
>>    uint8_t *p;
>> +    int ret;
>> +
>> +    ret = ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL,
>> RTMP_PT_BYTES_READ,
>> +                                ts, 4);
>> +    if (ret < 0)
>> +        return ret;
>>
>> -    ff_rtmp_packet_create(&pkt, RTMP_NETWORK_CHANNEL,
>> RTMP_PT_BYTES_READ, ts, 4);
>>    p = pkt.data;
>>    bytestream_put_be32(&p, rt->bytes_read);
>>    ff_rtmp_packet_write(rt->**stream, &pkt, rt->chunk_size,
>> rt->prev_pkt[1]);
>>    ff_rtmp_packet_destroy(&pkt);
>> +
>> +    return 0;
>> }
>>
>> //TODO: Move HMAC code somewhere. Eventually.
>> @@ -599,6 +675,7 @@ static int rtmp_parse_result(URLContext *s,
>> RTMPContext *rt, RTMPPacket *pkt)
>> {
>>    int i, t;
>>    const uint8_t *data_end = pkt->data + pkt->data_size;
>> +    int ret;
>>
>> #ifdef DEBUG
>>    ff_rtmp_packet_dump(s, pkt);
>> @@ -622,8 +699,11 @@ static int rtmp_parse_result(URLContext *s,
>> RTMPContext *rt, RTMPPacket *pkt)
>>        break;
>>    case RTMP_PT_PING:
>>        t = AV_RB16(pkt->data);
>> -        if (t == 6)
>> -            gen_pong(s, rt, pkt);
>> +        if (t == 6) {
>> +            ret = gen_pong(s, rt, pkt);
>> +            if (ret < 0)
>> +                return ret;
>> +        }
>>        break;
>>    case RTMP_PT_CLIENT_BW:
>>        if (pkt->data_size < 4) {
>> @@ -648,14 +728,22 @@ static int rtmp_parse_result(URLContext *s,
>> RTMPContext *rt, RTMPPacket *pkt)
>>            switch (rt->state) {
>>            case STATE_HANDSHAKED:
>>                if (!rt->is_input) {
>> -                    gen_release_stream(s, rt);
>> -                    gen_fcpublish_stream(s, rt);
>> +                    ret = gen_release_stream(s, rt);
>> +                    if (ret < 0)
>> +                        return ret;
>> +                    ret = gen_fcpublish_stream(s, rt);
>> +                    if (ret < 0)
>> +                        return ret;
>>
>
> Since this bloats the code quite a bit, you can write a bit more compactly
> like this:
>
> if ((ret = gen_release_stream(s, rt)) < 0)
>    return ret;
>
> That saves a few lines, if you prefer that. (I guess the same can be
> applied at most of the places in this file. In the gen_*() functions, it's
> not as bad since it's just one instance per function, but for places where
> this, when the reader rather would try to focus on the higher level
> aspects, shortening the code might make it more readable.)
>
>
>                     rt->state = STATE_RELEASING;
>>                } else {
>> -                    gen_server_bw(s, rt);
>> +                    ret = gen_server_bw(s, rt);
>> +                    if (ret < 0)
>> +                        return ret;
>>                    rt->state = STATE_CONNECTING;
>>                }
>> -                gen_create_stream(s, rt);
>> +                ret = gen_create_stream(s, rt);
>> +                if (ret < 0)
>> +                    return ret;
>>                break;
>>            case STATE_FCPUBLISH:
>>                rt->state = STATE_CONNECTING;
>> @@ -679,9 +767,13 @@ static int rtmp_parse_result(URLContext *s,
>> RTMPContext *rt, RTMPPacket *pkt)
>>                    rt->main_channel_id = av_int2double(AV_RB64(pkt->**data
>> + 21));
>>                }
>>                if (rt->is_input) {
>> -                    gen_play(s, rt);
>> +                    ret = gen_play(s, rt);
>> +                    if (ret < 0)
>> +                        return ret;
>>                } else {
>> -                    gen_publish(s, rt);
>> +                    ret = gen_publish(s, rt);
>> +                    if (ret < 0)
>> +                        return ret;
>>                }
>>                rt->state = STATE_READY;
>>                break;
>> @@ -711,7 +803,9 @@ static int rtmp_parse_result(URLContext *s,
>> RTMPContext *rt, RTMPPacket *pkt)
>>            if (!t && !strcmp(tmpstr, "NetStream.Play.**UnpublishNotify"))
>> rt->state = STATE_STOPPED;
>>            if (!t && !strcmp(tmpstr, "NetStream.Publish.Start"))
>> rt->state = STATE_PUBLISHING;
>>        } else if (!memcmp(pkt->data, "\002\000\010onBWDone", 11)) {
>> -            gen_check_bw(s, rt);
>> +            ret = gen_check_bw(s, rt);
>> +            if (ret < 0)
>> +                return ret;
>>        }
>>        break;
>>    }
>> @@ -754,14 +848,16 @@ static int get_packet(URLContext *s, int for_header)
>>        rt->bytes_read += ret;
>>        if (rt->bytes_read > rt->last_bytes_read + rt->client_report_size)
>> {
>>            av_log(s, AV_LOG_DEBUG, "Sending bytes read report\n");
>> -            gen_bytes_read(s, rt, rpkt.timestamp + 1);
>> +            ret = gen_bytes_read(s, rt, rpkt.timestamp + 1);
>> +            if (ret < 0)
>> +                return ret;
>>            rt->last_bytes_read = rt->bytes_read;
>>        }
>>
>>        ret = rtmp_parse_result(s, rt, &rpkt);
>>        if (ret < 0) {//serious error in current packet
>>            ff_rtmp_packet_destroy(&rpkt);
>> -            return -1;
>> +            return ret;
>>        }
>>        if (rt->state == STATE_STOPPED) {
>>            ff_rtmp_packet_destroy(&rpkt);
>> @@ -825,16 +921,23 @@ static int get_packet(URLContext *s, int for_header)
>> static int rtmp_close(URLContext *h)
>> {
>>    RTMPContext *rt = h->priv_data;
>> +    int ret;
>>
>>    if (!rt->is_input) {
>>        rt->flv_data = NULL;
>>        if (rt->out_pkt.data_size)
>>            ff_rtmp_packet_destroy(&rt->**out_pkt);
>> -        if (rt->state > STATE_FCPUBLISH)
>> -            gen_fcunpublish_stream(h, rt);
>> +        if (rt->state > STATE_FCPUBLISH) {
>> +            ret = gen_fcunpublish_stream(h, rt);
>> +            if (ret < 0)
>> +                return ret;
>> +        }
>> +    }
>> +    if (rt->state > STATE_HANDSHAKED) {
>> +        ret = gen_delete_stream(h, rt);
>> +        if (ret < 0)
>> +            return ret;
>>    }
>> -    if (rt->state > STATE_HANDSHAKED)
>> -        gen_delete_stream(h, rt);
>>
>
> This is not right. If an allocation fails in any of these functions, you'd
> just return from rtmp_close and leak the allocated data structures and the
> nested urlprotocol. So for this case, it's better to just ignore any
> failure in these and run the function to the end. If you want to, you might
> want to keep track of the return value in order to indicate that not
> everything worked out fine. But I'm not sure if there's any code anywhere
> that checks the return value from ffurl_close() (what would one do in that
> case anyway?). But make sure all the allocated memory is freed in any case.


I'm sorry but I don't understand what you mean above. Could you rephrase
it, please?

>
>
>
>>    av_freep(&rt->flv_data);
>>    ffurl_close(rt->stream);
>> @@ -871,12 +974,15 @@ static int rtmp_open(URLContext *s, const char
>> *uri, int flags)
>>    if (ffurl_open(&rt->stream, buf, AVIO_FLAG_READ_WRITE,
>>                   &s->interrupt_callback, NULL) < 0) {
>>        av_log(s , AV_LOG_ERROR, "Cannot open connection %s\n", buf);
>> +        ret = AVERROR(EIO);
>>        goto fail;
>>    }
>>
>
> I'd rather use if ((ret = ffurl_open()) < 0) here, so that you pass the
> actual error along, instead of inventing a new one.
>
>
>
>>    rt->state = STATE_START;
>> -    if (rtmp_handshake(s, rt))
>> +    if (rtmp_handshake(s, rt)) {
>> +        ret = AVERROR(EIO);
>>        goto fail;
>> +    }
>>
>
> This might be ok. Or does rtmp_handshake return an error code we'd like to
> pass on?
>
>
>     rt->chunk_size = 128;
>>    rt->state = STATE_HANDSHAKED;
>> @@ -886,8 +992,8 @@ static int rtmp_open(URLContext *s, const char *uri,
>> int flags)
>>
>>    rt->app = av_malloc(APP_MAX_LENGTH);
>>    if (!rt->app) {
>> -        rtmp_close(s);
>> -        return AVERROR(ENOMEM);
>> +        ret = AVERROR(ENOMEM);
>> +        goto fail;
>>    }
>>
>
> Hmm, except for unifying things, does this make any difference - I don't
> think so. But it's ok to keep it included (add a mention about unifying
> error handling in the commit message).
>
>
>     //extract "app" part from path
>> @@ -922,8 +1028,8 @@ static int rtmp_open(URLContext *s, const char *uri,
>> int flags)
>>    if (!rt->playpath) {
>>        rt->playpath = av_malloc(PLAYPATH_MAX_LENGTH)**;
>>        if (!rt->playpath) {
>> -            rtmp_close(s);
>> -            return AVERROR(ENOMEM);
>> +            ret = AVERROR(ENOMEM);
>> +            goto fail;
>>        }
>>
>>        if (!strchr(fname, ':') &&
>> @@ -960,13 +1066,17 @@ static int rtmp_open(URLContext *s, const char
>> *uri, int flags)
>>
>>    av_log(s, AV_LOG_DEBUG, "Proto = %s, path = %s, app = %s, fname =
>> %s\n",
>>           proto, path, rt->app, rt->playpath);
>> -    gen_connect(s, rt);
>> +    if (gen_connect(s, rt) < 0) {
>> +        ret = AVERROR(ENOMEM);
>> +        goto fail;
>> +    }
>>
>
> Same here as above (and probably in a few other places as well), passing
> the actual returned value along is better.
>
>
>     do {
>>        ret = get_packet(s, 1);
>>    } while (ret == EAGAIN);
>> -    if (ret < 0)
>> +    if (ret < 0) {
>>        goto fail;
>> +    }
>>
>
> Unrelated cosmetic change
>
>
>     if (rt->is_input) {
>>        // generate FLV header for demuxer
>> @@ -987,7 +1097,7 @@ static int rtmp_open(URLContext *s, const char *uri,
>> int flags)
>>
>> fail:
>>    rtmp_close(s);
>> -    return AVERROR(EIO);
>> +    return ret;
>> }
>>
>> static int rtmp_read(URLContext *s, uint8_t *buf, int size)
>> @@ -1024,6 +1134,7 @@ static int rtmp_write(URLContext *s, const uint8_t
>> *buf, int size)
>>    int pktsize, pkttype;
>>    uint32_t ts;
>>    const uint8_t *buf_temp = buf;
>> +    int ret;
>>
>>    do {
>>        if (rt->skip_bytes) {
>> @@ -1059,7 +1170,11 @@ static int rtmp_write(URLContext *s, const uint8_t
>> *buf, int size)
>>            }
>>
>>            //this can be a big packet, it's better to send it right here
>> -            ff_rtmp_packet_create(&rt->**out_pkt, RTMP_SOURCE_CHANNEL,
>> pkttype, ts, pktsize);
>> +            ret = ff_rtmp_packet_create(&rt->**out_pkt,
>> RTMP_SOURCE_CHANNEL,
>> +                                        pkttype, ts, pktsize);
>> +            if (ret < 0)
>> +                return ret;
>> +
>>            rt->out_pkt.extra = rt->main_channel_id;
>>            rt->flv_data = rt->out_pkt.data;
>>
>> --
>> 1.7.10.2
>>
>
>
> // Martin
> ______________________________**_________________
> libav-devel mailing list
> libav-devel at libav.org
> https://lists.libav.org/**mailman/listinfo/libav-devel<https://lists.libav.org/mailman/listinfo/libav-devel>
>



-- 
Best regards,
Samuel Pitoiset.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.libav.org/pipermail/libav-devel/attachments/20120523/faf99366/attachment-0001.html>


More information about the libav-devel mailing list