[05/13] rtpdec_h264: Remove an unnecessary check
diff mbox

Message ID 1424378051-6607-5-git-send-email-martin@martin.st
State Committed
Commit a335ed767161c6da2815371177cfd5e40f78e5b7
Headers show

Commit Message

Martin Storsjö Feb. 19, 2015, 8:34 p.m. 
The if statement just above checks the same, so there's no possibility
that src_len ends up negative.
---
 libavformat/rtpdec_h264.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Diego Biurrun Feb. 19, 2015, 10:06 p.m. | #1 
On Thu, Feb 19, 2015 at 10:34:03PM +0200, Martin Storsjö wrote:
> The if statement just above checks the same, so there's no possibility
> that src_len ends up negative.

I agree about the if-statement, but it seems src_len can end up negative.
Here's a shortened version of the block:

    while (src_len > 2) {
        uint16_t nal_size = AV_RB16(src);
        src_len -= 2;
        src_len -= nal_size;
        if (src_len < 0)
            av_log(ctx, AV_LOG_ERROR,
                   "Consumed more bytes than we got! (%d)\n", src_len);

Looks like src_len can end up negative to me ...

Diego
Martin Storsjö Feb. 20, 2015, 10:11 a.m. | #2 
On Thu, 19 Feb 2015, Diego Biurrun wrote:

> On Thu, Feb 19, 2015 at 10:34:03PM +0200, Martin Storsjö wrote:
>> The if statement just above checks the same, so there's no possibility
>> that src_len ends up negative.
>
> I agree about the if-statement, but it seems src_len can end up negative.
> Here's a shortened version of the block:
>
>    while (src_len > 2) {
>        uint16_t nal_size = AV_RB16(src);
>        src_len -= 2;
>        src_len -= nal_size;
>        if (src_len < 0)
>            av_log(ctx, AV_LOG_ERROR,
>                   "Consumed more bytes than we got! (%d)\n", src_len);
>
> Looks like src_len can end up negative to me ...

Oh, right.

So, fine if I change the commit message like this?

---
rtpdec_h264: Remove an unnecessary check

If src_len is too small for nal_size, we already print a warning above, 
and the next step is to check the while loop condition anyway, so this one 
serves no purpose.
---

// Martin
Diego Biurrun Feb. 20, 2015, 3:39 p.m. | #3 
On Fri, Feb 20, 2015 at 12:11:10PM +0200, Martin Storsjö wrote:
> On Thu, 19 Feb 2015, Diego Biurrun wrote:
> >On Thu, Feb 19, 2015 at 10:34:03PM +0200, Martin Storsjö wrote:
> >>The if statement just above checks the same, so there's no possibility
> >>that src_len ends up negative.
> >
> >I agree about the if-statement, but it seems src_len can end up negative.
> >Here's a shortened version of the block:
> >
> >   while (src_len > 2) {
> >       uint16_t nal_size = AV_RB16(src);
> >       src_len -= 2;
> >       src_len -= nal_size;
> >       if (src_len < 0)
> >           av_log(ctx, AV_LOG_ERROR,
> >                  "Consumed more bytes than we got! (%d)\n", src_len);
> >
> >Looks like src_len can end up negative to me ...
> 
> Oh, right.
> 
> So, fine if I change the commit message like this?
> 
> ---
> rtpdec_h264: Remove an unnecessary check
> 
> If src_len is too small for nal_size, we already print a warning above, and
> the next step is to check the while loop condition anyway, so this one
> serves no purpose.

I'm fine with that or even a much more terse version, but we shouldn't
make any false claims in the log message :)

Diego

Patch
diff mbox 

diff --git a/libavformat/rtpdec_h264.c b/libavformat/rtpdec_h264.c
index 5b87529..5ffb4c4 100644
--- a/libavformat/rtpdec_h264.c
+++ b/libavformat/rtpdec_h264.c
@@ -214,10 +214,6 @@  static int h264_handle_packet_stap_a(AVFormatContext *ctx, AVPacket *pkt,
             // eat what we handled
             src     += nal_size;
             src_len -= nal_size;
-
-            if (src_len < 0)
-                av_log(ctx, AV_LOG_ERROR,
-                       "Consumed more bytes than we got! (%d)\n", src_len);
         }
 
         if (pass == 0) {