[FFmpeg-devel,v4] lavf/h264: add support for h264 video from Arecont camera, fixes ticket #5154

Submitted by Shivam Goyal on May 9, 2019, 6:15 p.m.

Details

Message ID 28c147610fff100a656b2f1bae0ebbde@iitk.ac.in
State New
Headers show

Commit Message

Shivam Goyal May 9, 2019, 6:15 p.m.
The patch is for ticket #5154. 

Support for h264 stream from Arecont Camera, The video contains a http
header at several places in the stream, which makes it not possible to
demux, the structure of the http header is- 

--fbdr 
Content-Type: video/H.264I 
ETag: Channel=1 

( there are two CR+LF characters at the end ) 

I also detected the stream by detecting this header. For the video to
play, i removed the header. The function arecont_find_sign() searches
for the signature  and returns its position, (-1 when not found ). 

Also, in the function read_raw_arecont_h264_packet, i used two variables
to store data, the 'pkt->data' and 'data'. I first filled the 'data' and
find the position of http header ( if it is present) and copied the
bytes before the position of the http header, skipped the header, and
again did the same thing for rest of the bytes. also as the packet can
contain partial header at the beginning and end of the pkt, i took extra
size to read from the input stream 

Please review 

Thank You 

Shivam Goyal

Comments

Reimar Döffinger May 10, 2019, 10:57 p.m.
On Thu, May 09, 2019 at 11:45:59PM +0530, Shivam Goyal wrote:
> @@ -117,4 +120,128 @@ static int h264_probe(const AVProbeData *p)
>      return 0;
>  }
>
> +static const uint8_t arecont_sign[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A};

I admit I was more thinking of either
static const uint8_t arecont_sign[] = "--fbdr\r\n";
which ends up 1 byte too long, or
static const uint8_t arecont_sign[8] = "--fbdr\r\n";

Though there is also the option to go for
static const uint64_t arecont_sign = AV_RL64("--fbdr\r\n");
or similar.

> +static int arecont_find_sign(unsigned char *data, int size)

You should be consistent with the types even if they are essentially
the same.
i.e. uint8_t instead of unsigned char *
Also const, since this function does/should not modify "data".

> +    int sign_size = sizeof(arecont_sign) / sizeof(arecont_sign[0]);

First, this is the expression of number of elements, so the
division part is semantically wrong.
Also it's pointless because it will be 1 always.

> +    j = memchr(data, arecont_sign[0], size);
> +    while (j != NULL && size - sign_size >= (j - data)) {
> +        if (!memcmp(arecont_sign, j, sign_size))
> +            return (j - data);
> +        if (size - sign_size == (j - data))
> +            return -1;
> +        j = memchr(data + (j - data) + 1, arecont_sign[0], size - (j - data));
> +    }

I know I brought this memchr up, but did you do any benchmarks?
Unless you have actual evidence of a speed problem AND can
show that your solution actually makes it faster, I'd
suggest to go with the simplest possible solution.

> +    data = av_malloc(size);
> +    ret = avio_read_partial(s->pb, data, size);
> +    if (ret < 0) {
> +        av_free(data);
> +        av_packet_unref(pkt);
> +        return ret;
> +    }
> +    if (pos <= ARECONT_H264_MIME_SIZE) {
> +        avio_seek(s->pb, 0, SEEK_SET);
> +        start = pos;
> +    } else {
> +        avio_seek(s->pb, pos - ARECONT_H264_MIME_SIZE, SEEK_SET);
> +        start = ARECONT_H264_MIME_SIZE;
> +    }
> +    ret = avio_read_partial(s->pb, data, size);
> +    if (ret < 0 || start >= ret) {
> +        av_free(data);
> +        av_packet_unref(pkt);
> +        return ret;
> +    }

Unfortunately I still have no idea what that code is meant to do.
First there is no point in allocating "data" when you have
pkt->data already (yes, it would mean using memmove instead
of memcpy later on, but that seems to be about it).
Then the code reads TWICE into the same buffer for some reason,
the first read seems completely pointless?
Also the only point of the _partial variant of the read function
is reducing latency, however the avio_seek is likely to be
quite a bad hit for latency that this really seems like
premature optimization.
Also the avio_seek means this demuxer might not work at all
if the stream is not seekable (might since I don't know
if we maybe do enough buffering nowadays - but if the code
relies on that there should be a check).

> +        if ((j = arecont_find_sign(data + i, ret - i)) >= 0) {

Very personal dislike but:
please just don't put assignments into the if.

> +            k = 0;
> +            for (w = j + sign_size; w + 1 < ret; w++) {

So what exactly happens when you actually end up hitting
that loop end condition?
It seems to me you then just leave the whole thing in,
even though you should have removed it?

> +        } else
> +            break;

Inverting the condition and putting the 1-line case in the
"if" instead of the "else" is much better for readability.
In this case it also saves you 1 indentation level.

On the general approach:
you are scanning a whole, potentially many, many GB large video
file for a 8-character string.
A false positive has a REALLY high likelihood.
I think this needs to be changed into a more clever approach,
that actually knows where these strings can appear and removes
them in a more targeted way...

Best regards,
Reimar Döffinger
Shivam Goyal May 11, 2019, 7:43 a.m.
On 11-05-2019 04:27, Reimar Döffinger wrote:

> On Thu, May 09, 2019 at 11:45:59PM +0530, Shivam Goyal wrote: 
> 
>> @@ -117,4 +120,128 @@ static int h264_probe(const AVProbeData *p)
>> return 0;
>> }
>> 
>> +static const uint8_t arecont_sign[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A};
> 
> I admit I was more thinking of either
> static const uint8_t arecont_sign[] = "--fbdr\r\n";
> which ends up 1 byte too long, or
> static const uint8_t arecont_sign[8] = "--fbdr\r\n";
> 
> Though there is also the option to go for
> static const uint64_t arecont_sign = AV_RL64("--fbdr\r\n");
> or similar.

   Okay, I will change it. 

>> +static int arecont_find_sign(unsigned char *data, int size)
> 
> You should be consistent with the types even if they are essentially
> the same.
> i.e. uint8_t instead of unsigned char *
> Also const, since this function does/should not modify "data".

   Will change it too, 

>> +    int sign_size = sizeof(arecont_sign) / sizeof(arecont_sign[0]);
> 
> First, this is the expression of number of elements, so the
> division part is semantically wrong.
> Also it's pointless because it will be 1 always.

    Will use strlen() as i am changing the above. 

>> +    j = memchr(data, arecont_sign[0], size);
>> +    while (j != NULL && size - sign_size >= (j - data)) {
>> +        if (!memcmp(arecont_sign, j, sign_size))
>> +            return (j - data);
>> +        if (size - sign_size == (j - data))
>> +            return -1;
>> +        j = memchr(data + (j - data) + 1, arecont_sign[0], size - (j - data));
>> +    }
> 
> I know I brought this memchr up, but did you do any benchmarks?
> Unless you have actual evidence of a speed problem AND can
> show that your solution actually makes it faster, I'd
> suggest to go with the simplest possible solution.

 Yeah, I checked the both approaches and perform test to compare time.
and found the previous one is good for shorter 
 strings (length < 300). and the newer version is good for strings
larger than this. The newer version is 4 microseconds faster 
 in our case(length ~ 1100). As the diff is small, so i think the
simpler version is better. 

>> +    data = av_malloc(size);
>> +    ret = avio_read_partial(s->pb, data, size);
>> +    if (ret < 0) {
>> +        av_free(data);
>> +        av_packet_unref(pkt);
>> +        return ret;
>> +    }
>> +    if (pos <= ARECONT_H264_MIME_SIZE) {
>> +        avio_seek(s->pb, 0, SEEK_SET);
>> +        start = pos;
>> +    } else {
>> +        avio_seek(s->pb, pos - ARECONT_H264_MIME_SIZE, SEEK_SET);
>> +        start = ARECONT_H264_MIME_SIZE;
>> +    }
>> +    ret = avio_read_partial(s->pb, data, size);
>> +    if (ret < 0 || start >= ret) {
>> +        av_free(data);
>> +        av_packet_unref(pkt);
>> +        return ret;
>> +    }
> 
> Unfortunately I still have no idea what that code is meant to do.
> First there is no point in allocating "data" when you have
> pkt->data already (yes, it would mean using memmove instead
> of memcpy later on, but that seems to be about it).
> Then the code reads TWICE into the same buffer for some reason,
> the first read seems completely pointless?
> Also the only point of the _partial variant of the read function
> is reducing latency, however the avio_seek is likely to be
> quite a bad hit for latency that this really seems like
> premature optimization.
> Also the avio_seek means this demuxer might not work at all
> if the stream is not seekable (might since I don't know
> if we maybe do enough buffering nowadays - but if the code
> relies on that there should be a check).

    What i did in the above code is that as the packet can contain
partial http header at the start or end of the pkt, which needs to be
removed, i seeked backward 100 bytes( or at the start of the file if
necessory) and the i did the same to remove the partial http header at
the end, i seeked forward 100 bytes extra. 

                  start                      end 
stream..............^.........................^.................... 
             |_____________|             |_________| 
                    |                         | 
              http header                http header 

I first filled the "data" and then searched for the http header, copied
the bytes before position of http header from "data" to pkt->data. And
then skipped the http header and again searched for another http header
in "data" and did the same thing. Also, the code is reading twice
because the first is just if any errors come( like file ended). because
after that the avio_seek() seek stream backwards and if it would do,
then error can't be detected. 

Yeah, i realized that the avio_seek() is not a good idea. One another
approach i thought is that to store 100 bytes (or 70) of the current
iteration for the next iteration. In this way we don't have to use
avio_seek.also there would not be any need of reading twice. Please
suggest if it is right. (Sorry, if i am asking too much doubts). 

>> +        if ((j = arecont_find_sign(data + i, ret - i)) >= 0) {
> 
> Very personal dislike but:
> please just don't put assignments into the if.

   Okay, would change this. 

>> +            k = 0;
>> +            for (w = j + sign_size; w + 1 < ret; w++) {
> 
> So what exactly happens when you actually end up hitting
> that loop end condition?
> It seems to me you then just leave the whole thing in,
> even though you should have removed it?

 When the http header is not found on the "data" then the loop ends, or
if it is found then the bytes before the position of http-header are
copied to pkt->data and then the http-header is skipped and loop again
runs. 

>> +        } else
>> +            break;
> 
> Inverting the condition and putting the 1-line case in the
> "if" instead of the "else" is much better for readability.
> In this case it also saves you 1 indentation level.

  Okay, would change this too. 

> On the general approach:
> you are scanning a whole, potentially many, many GB large video
> file for a 8-character string.
> A false positive has a REALLY high likelihood.
> I think this needs to be changed into a more clever approach,
> that actually knows where these strings can appear and removes
> them in a more targeted way...

Yeah this would be slow for large files, I also tried finding out if
there is any relation between positions of the http-header but didn't
found.

  Thanks for the review 
  Shivam Goyal

Patch hide | download patch | download mbox

From deb859ff488c500b7e6005e350f04c200f2853a4 Mon Sep 17 00:00:00 2001
From: Shivam Goyal <shivamgoyal1506@outlook.com>
Date: Thu, 9 May 2019 23:18:13 +0530
Subject: [PATCH] lavf/h264: Add support for h264 video from Arecont Camera,
 Fixes ticket #5154

---
 Changelog                |   1 +
 libavformat/Makefile     |   1 +
 libavformat/allformats.c |   1 +
 libavformat/h264dec.c    | 127 +++++++++++++++++++++++++++++++++++++++
 libavformat/version.h    |   4 +-
 5 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/Changelog b/Changelog
index a3fa0c14a2..0733e360fc 100644
--- a/Changelog
+++ b/Changelog
@@ -26,6 +26,7 @@  version <next>:
 - lscr decoder
 - lagfun filter
 - asoftclip filter
+- Support for raw h264 video from Arecont camera
 
 
 version 4.1:
diff --git a/libavformat/Makefile b/libavformat/Makefile
index 99be60d184..be0f286870 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -99,6 +99,7 @@  OBJS-$(CONFIG_APTX_MUXER)                += rawenc.o
 OBJS-$(CONFIG_APTX_HD_DEMUXER)           += aptxdec.o rawdec.o
 OBJS-$(CONFIG_APTX_HD_MUXER)             += rawenc.o
 OBJS-$(CONFIG_AQTITLE_DEMUXER)           += aqtitledec.o subtitles.o
+OBJS-$(CONFIG_ARECONT_H264_DEMUXER)      += h264dec.o rawdec.o
 OBJS-$(CONFIG_ASF_DEMUXER)               += asfdec_f.o asf.o asfcrypt.o \
                                             avlanguage.o
 OBJS-$(CONFIG_ASF_O_DEMUXER)             += asfdec_o.o asf.o asfcrypt.o \
diff --git a/libavformat/allformats.c b/libavformat/allformats.c
index d316a0529a..fff8083172 100644
--- a/libavformat/allformats.c
+++ b/libavformat/allformats.c
@@ -60,6 +60,7 @@  extern AVOutputFormat ff_aptx_muxer;
 extern AVInputFormat  ff_aptx_hd_demuxer;
 extern AVOutputFormat ff_aptx_hd_muxer;
 extern AVInputFormat  ff_aqtitle_demuxer;
+extern AVInputFormat  ff_arecont_h264_demuxer;
 extern AVInputFormat  ff_asf_demuxer;
 extern AVOutputFormat ff_asf_muxer;
 extern AVInputFormat  ff_asf_o_demuxer;
diff --git a/libavformat/h264dec.c b/libavformat/h264dec.c
index 199e87fbfa..99d7f85b40 100644
--- a/libavformat/h264dec.c
+++ b/libavformat/h264dec.c
@@ -28,6 +28,9 @@ 
 #define MAX_SPS_COUNT          32
 #define MAX_PPS_COUNT         256
 
+#define ARECONT_H264_MIME_SIZE  100
+#define RAW_PACKET_SIZE         1024
+
 static int h264_probe(const AVProbeData *p)
 {
     uint32_t code = -1;
@@ -117,4 +120,128 @@  static int h264_probe(const AVProbeData *p)
     return 0;
 }
 
+static const uint8_t arecont_sign[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A};
+
+static int arecont_find_sign(unsigned char *data, int size)
+{
+    unsigned char *j;
+    int sign_size = sizeof(arecont_sign) / sizeof(arecont_sign[0]);
+
+    j = memchr(data, arecont_sign[0], size);
+    while (j != NULL && size - sign_size >= (j - data)) {
+        if (!memcmp(arecont_sign, j, sign_size))
+            return (j - data);
+        if (size - sign_size == (j - data))
+            return -1;
+        j = memchr(data + (j - data) + 1, arecont_sign[0], size - (j - data));
+    }
+    return -1;
+}
+
+static int arecont_h264_probe(const AVProbeData *p)
+{
+    int ret = h264_probe(p);
+
+    if (!ret)
+        return 0;
+    if (arecont_find_sign(p->buf, p->buf_size) >= 0)
+        return ret + 1;
+    return 0;
+}
+
+static int read_raw_arecont_h264_packet(AVFormatContext *s, AVPacket *pkt)
+{
+    int ret, size, start, end, new_size = 0, i = 0, j = 0, k = 0, w = 0;
+    uint8_t *data;
+    int64_t pos;
+    int sign_size = sizeof(arecont_sign) / sizeof(arecont_sign[0]);
+
+    //Extra to find the http header
+    size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE;
+
+    if (av_new_packet(pkt, size) < 0)
+        return AVERROR(ENOMEM);
+
+    pkt->pos = avio_tell(s->pb);
+    pkt->stream_index = 0;
+    pos = avio_tell(s->pb);
+
+    data = av_malloc(size);
+    ret = avio_read_partial(s->pb, data, size);
+    if (ret < 0) {
+        av_free(data);
+        av_packet_unref(pkt);
+        return ret;
+    }
+    if (pos <= ARECONT_H264_MIME_SIZE) {
+        avio_seek(s->pb, 0, SEEK_SET);
+        start = pos;
+    } else {
+        avio_seek(s->pb, pos - ARECONT_H264_MIME_SIZE, SEEK_SET);
+        start = ARECONT_H264_MIME_SIZE;
+    }
+    ret = avio_read_partial(s->pb, data, size);
+    if (ret < 0 || start >= ret) {
+        av_free(data);
+        av_packet_unref(pkt);
+        return ret;
+    }
+    if (start + RAW_PACKET_SIZE > ret) {
+        end = ret - 1;
+    } else
+        end = start + RAW_PACKET_SIZE - 1;
+
+
+    // Search and remove the http header
+    for (;;) {
+        if ((j = arecont_find_sign(data + i, ret - i)) >= 0) {
+            k = 0;
+            for (w = j + sign_size; w + 1 < ret; w++) {
+                if (!(data[w] == 0x0D && data[w + 1] == 0x0A))
+                    continue;
+                k++;
+                if (k < 3)
+                    continue;
+                if (j < start) {
+                    i = w + 2;
+                    break;
+                }
+                memcpy(pkt->data + new_size, data + FFMAX(i, start), FFMIN(j, end + 1) - FFMAX(i, start));
+                new_size +=  FFMIN(j, end + 1) - FFMAX(i, start);
+                i = w + 2;
+                break;
+            }
+        } else
+            break;
+    }
+    if (i < end) {
+        memcpy(pkt->data + new_size, data + FFMAX(i, start), end + 1 - FFMAX(i, start));
+        new_size += end + 1 - FFMAX(i, start);
+    }
+    avio_seek(s->pb, pos + end - start + 1, SEEK_SET);
+    av_shrink_packet(pkt, new_size);
+    av_free(data);
+    return new_size;
+}
+
 FF_DEF_RAWVIDEO_DEMUXER(h264, "raw H.264 video", h264_probe, "h26l,h264,264,avc", AV_CODEC_ID_H264)
+
+static const AVClass arecont_h264_class = {
+    .class_name = "arecont_h264_dec",
+    .item_name  = av_default_item_name,
+    .option     = ff_rawvideo_options,
+    .version    = LIBAVUTIL_VERSION_INT,
+};
+
+AVInputFormat ff_arecont_h264_demuxer = {
+    .name           = "arecont_h264",
+    .long_name      = NULL_IF_CONFIG_SMALL("raw H.264 video from Arecont camera"),
+    .read_probe     = arecont_h264_probe,
+    .read_header    = ff_raw_video_read_header,
+    .read_packet    = read_raw_arecont_h264_packet,
+    .extensions     = "",
+    .flags          = AVFMT_GENERIC_INDEX,
+    .raw_codec_id   = AV_CODEC_ID_H264,
+    .priv_data_size = sizeof(FFRawVideoDemuxerContext),
+    .priv_class     = &arecont_h264_class,
+};
\ No newline at end of file
diff --git a/libavformat/version.h b/libavformat/version.h
index 150a72e27d..52dd95f5c6 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,8 +32,8 @@ 
 // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  58
-#define LIBAVFORMAT_VERSION_MINOR  27
-#define LIBAVFORMAT_VERSION_MICRO 103
+#define LIBAVFORMAT_VERSION_MINOR  28
+#define LIBAVFORMAT_VERSION_MICRO 100
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
                                                LIBAVFORMAT_VERSION_MINOR, \
-- 
2.21.0