Message ID | ce97e055601a4be81c21846e96cf368a@iitk.ac.in |
---|---|
State | Superseded |
Headers | show |
On 07-05-2019 10:05, Shivam Goyal wrote: > The patch is for ticket #5154. > > I have improved the patch as suggested. > > Please review. > > Thank you, > > Shivam Goyal Ping, Please review. Thank you Shivam Goyal
On Tue, May 07, 2019 at 10:05:12AM +0530, Shivam Goyal wrote: > +static int arecont_h264_probe(const AVProbeData *p) > +{ > + int i, j, k, o = 0; > + int ret = h264_probe(p); > + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A}; Should be "static const" instead of just "const". Also this seems to be just "--fbdr\r\n"? > + if (p->buf[i] == id[0] && !memcmp(id, p->buf + i, 8)) > + o++; Is that optimization really helping much? If you want to speed it up, I suspect it would be more useful to either go for AV_RL64 or memchr() or a combination. Also, sizeof() instead of hard-coding the 8. > + if (o >= 1) > + return ret + 1; Since you only check for >= 1 you could have aborted the scanning loop in the first match... > +static int read_raw_arecont_h264_packet(AVFormatContext *s, AVPacket *pkt) > +{ > + int ret, size, start, end, new_size = 0, i, j, k, w = 0; > + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72}; "static", however this seems the same data (though 2 shorter). I'd suggest defining the signature just once. > + uint8_t *data; > + int64_t pos; > + > + //Extra to find the http header > + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; > + data = av_malloc(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); > + ret = avio_read_partial(s->pb, data, size); > + if (ret < 0) { > + 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_packet_unref(pkt); > + return ret; > + } You need to document more what you are doing here. And even more importantly why you are using avio_read_partial. And why you allocate both a packet and a separate "data" with the same size. And why not use av_get_packet. > + if (i >= start && j + 1 > start && j + 1 <= end) { > + memcpy(pkt->data + new_size, data + start, i - start); > + new_size += i - start; > + memcpy(pkt->data + new_size, data + j + 2, end - j - 1); > + new_size += end - j - 1; > + w = 1; > + break; > + } else if (i < start && j + 1 >= start && j + 1 < end) { > + memcpy(pkt->data + new_size, data + j + 2, end - j -1); > + new_size += end - j - 1; > + w = 1; > + break; > + } else if (j + 1 > end && i > start && i <= end) { > + memcpy(pkt->data + new_size, data + start, i - start); > + new_size += i - start; > + w = 1; > + break; > + } With some comments I might be able to review without spending a lot of time reverse-engineering this... Best regards, Reimar Döffinger
On Tue, May 07, 2019 at 10:05:12AM +0530, Shivam Goyal wrote: > The patch is for ticket #5154. > > I have improved the patch as suggested. > > Please review. > > Thank you, > > Shivam Goyal > Changelog | 1 > libavformat/Makefile | 1 > libavformat/allformats.c | 1 > libavformat/h264dec.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++ > libavformat/version.h | 4 - > 5 files changed, 126 insertions(+), 2 deletions(-) > 34932cf36d17537b8fc34642e92cc1fff6ad481e add_arecont_h264_support_v3.patch > From 2aa843626f939218179d3ec252f76f9991c33ed6 Mon Sep 17 00:00:00 2001 > From: Shivam Goyal <shivamgoyal1506@outlook.com> > Date: Tue, 7 May 2019 10:01:15 +0530 > Subject: [PATCH] lavf/h264: Add support for h264 video from Arecont camera, > fixes ticket #5154 [...] > @@ -117,4 +120,122 @@ static int h264_probe(const AVProbeData *p) > return 0; > } > > +static int arecont_h264_probe(const AVProbeData *p) > +{ > + int i, j, k, o = 0; > + int ret = h264_probe(p); > + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A}; > + > + if (!ret) > + return 0; > + for (i = 0; i + 7 < p->buf_size; i++){ > + if (p->buf[i] == id[0] && !memcmp(id, p->buf + i, 8)) > + o++; > + } > + if (o >= 1) > + 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, j, k, w = 0; > + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72}; > + uint8_t *data; > + int64_t pos; > + > + //Extra to find the http header > + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; > + data = av_malloc(size); > + > + if (av_new_packet(pkt, size) < 0) > + return AVERROR(ENOMEM); memleak on error [...]
On 09-05-2019 01:15, Reimar Döffinger wrote: > On Tue, May 07, 2019 at 10:05:12AM +0530, Shivam Goyal wrote: > >> +static int arecont_h264_probe(const AVProbeData *p) >> +{ >> + int i, j, k, o = 0; >> + int ret = h264_probe(p); >> + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A}; > > Should be "static const" instead of just "const". > Also this seems to be just "--fbdr\r\n"? Okay, I would change it on next version, Yeah it is '--fbdr\r\n' The file from Arecont h264 contains http header starting from this, many times. > + if (p->buf[i] == id[0] && !memcmp(id, p->buf + i, 8)) > + o++; > Is that optimization really helping much? > If you want to speed it up, I suspect it would be more > useful to either go for AV_RL64 or memchr() or > a combination. > Also, sizeof() instead of hard-coding the 8. > > + if (o >= 1) > + return ret + 1; > Since you only check for >= 1 you could have aborted the > scanning loop in the first match... Yeah, this would be a great idea. I will change this. > +static int read_raw_arecont_h264_packet(AVFormatContext *s, AVPacket *pkt) > +{ > + int ret, size, start, end, new_size = 0, i, j, k, w = 0; > + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72}; > "static", however this seems the same data (though 2 shorter). > I'd suggest defining the signature just once. > > + uint8_t *data; > + int64_t pos; > + > + //Extra to find the http header > + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; > + data = av_malloc(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); > + ret = avio_read_partial(s->pb, data, size); > + if (ret < 0) { > + 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_packet_unref(pkt); > + return ret; > + } > You need to document more what you are doing here. > And even more importantly why you are using avio_read_partial. > And why you allocate both a packet and a separate "data" > with the same size. > And why not use av_get_packet. I saw the raw video demuxer, there it was using avio_read_partial. Because it is allowed to read less data then specified. > + if (i >= start && j + 1 > start && j + 1 <= end) { > + memcpy(pkt->data + new_size, data + start, i - start); > + new_size += i - start; > + memcpy(pkt->data + new_size, data + j + 2, end - j - 1); > + new_size += end - j - 1; > + w = 1; > + break; > + } else if (i < start && j + 1 >= start && j + 1 < end) { > + memcpy(pkt->data + new_size, data + j + 2, end - j -1); > + new_size += end - j - 1; > + w = 1; > + break; > + } else if (j + 1 > end && i > start && i <= end) { > + memcpy(pkt->data + new_size, data + start, i - start); > + new_size += i - start; > + w = 1; > + break; > + } > With some comments I might be able to review without > spending a lot of time reverse-engineering this... Okay, would add comments to this also. I also have another idea to solve this findind and removing the http header, but it would require to change RAW_PACKET_SIZE. So, just want to know, can i change this? if yes, then how much can i change this? Thanks for the review, Shivam Goyal
On 09-05-2019 02:16, Michael Niedermayer wrote: > On Tue, May 07, 2019 at 10:05:12AM +0530, Shivam Goyal wrote: > >> The patch is for ticket #5154. >> >> I have improved the patch as suggested. >> >> Please review. >> >> Thank you, >> >> Shivam Goyal > >> Changelog | 1 >> libavformat/Makefile | 1 >> libavformat/allformats.c | 1 >> libavformat/h264dec.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++ >> libavformat/version.h | 4 - >> 5 files changed, 126 insertions(+), 2 deletions(-) >> 34932cf36d17537b8fc34642e92cc1fff6ad481e add_arecont_h264_support_v3.patch >> From 2aa843626f939218179d3ec252f76f9991c33ed6 Mon Sep 17 00:00:00 2001 >> From: Shivam Goyal <shivamgoyal1506@outlook.com> >> Date: Tue, 7 May 2019 10:01:15 +0530 >> Subject: [PATCH] lavf/h264: Add support for h264 video from Arecont camera, >> fixes ticket #5154 > [...] > >> @@ -117,4 +120,122 @@ static int h264_probe(const AVProbeData *p) >> return 0; >> } >> >> +static int arecont_h264_probe(const AVProbeData *p) >> +{ >> + int i, j, k, o = 0; >> + int ret = h264_probe(p); >> + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A}; >> + >> + if (!ret) >> + return 0; >> + for (i = 0; i + 7 < p->buf_size; i++){ >> + if (p->buf[i] == id[0] && !memcmp(id, p->buf + i, 8)) >> + o++; >> + } >> + if (o >= 1) >> + 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, j, k, w = 0; >> + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72}; >> + uint8_t *data; >> + int64_t pos; >> + >> + //Extra to find the http header >> + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; >> + data = av_malloc(size); >> + >> + if (av_new_packet(pkt, size) < 0) >> + return AVERROR(ENOMEM); > > memleak on error I tested it on the file attached to that ticket, the error didn't come. Please, could you tell me how to solve this and why this error came.
On 09-05-2019 02:16, Michael Niedermayer wrote: > On Tue, May 07, 2019 at 10:05:12AM +0530, Shivam Goyal wrote: > >> The patch is for ticket #5154. >> >> I have improved the patch as suggested. >> >> Please review. >> >> Thank you, >> >> Shivam Goyal > >> Changelog | 1 >> libavformat/Makefile | 1 >> libavformat/allformats.c | 1 >> libavformat/h264dec.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++ >> libavformat/version.h | 4 - >> 5 files changed, 126 insertions(+), 2 deletions(-) >> 34932cf36d17537b8fc34642e92cc1fff6ad481e add_arecont_h264_support_v3.patch >> From 2aa843626f939218179d3ec252f76f9991c33ed6 Mon Sep 17 00:00:00 2001 >> From: Shivam Goyal <shivamgoyal1506@outlook.com> >> Date: Tue, 7 May 2019 10:01:15 +0530 >> Subject: [PATCH] lavf/h264: Add support for h264 video from Arecont camera, >> fixes ticket #5154 > [...] > >> @@ -117,4 +120,122 @@ static int h264_probe(const AVProbeData *p) >> return 0; >> } >> >> +static int arecont_h264_probe(const AVProbeData *p) >> +{ >> + int i, j, k, o = 0; >> + int ret = h264_probe(p); >> + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A}; >> + >> + if (!ret) >> + return 0; >> + for (i = 0; i + 7 < p->buf_size; i++){ >> + if (p->buf[i] == id[0] && !memcmp(id, p->buf + i, 8)) >> + o++; >> + } >> + if (o >= 1) >> + 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, j, k, w = 0; >> + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72}; >> + uint8_t *data; >> + int64_t pos; >> + >> + //Extra to find the http header >> + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; >> + data = av_malloc(size); >> + >> + if (av_new_packet(pkt, size) < 0) >> + return AVERROR(ENOMEM); > > memleak on error I have tested this on the file attached to the ticket, the error didn't came. Please, could you tell me how to solve this error and why it came. Thanks for the review Shivam Goyal
On Thu, May 09, 2019 at 07:28:20 +0530, Shivam Goyal wrote: > >> + //Extra to find the http header > >> + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; > >> + data = av_malloc(size); > >> + > >> + if (av_new_packet(pkt, size) < 0) > >> + return AVERROR(ENOMEM); > > > > memleak on error > > I have tested this on the file attached to the ticket, the error didn't > came. Please, could you tell me how to solve this error and why it came. It's a hypothetical case. You can see the memleak by code review, without the need for actually reproducing it with a real world test. You need to clean up your allocated memory in error paths as well. Please see many other places in the ffmpeg code. Moritz
On 09-05-2019 15:00, Moritz Barsnick wrote: > On Thu, May 09, 2019 at 07:28:20 +0530, Shivam Goyal wrote: + //Extra to find the http header > + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; > + data = av_malloc(size); > + > + if (av_new_packet(pkt, size) < 0) > + return AVERROR(ENOMEM); > memleak on error I have tested this on the file attached to the ticket, the error didn't came. Please, could you tell me how to solve this error and why it came. It's a hypothetical case. You can see the memleak by code review, without the need for actually reproducing it with a real world test. You need to clean up your allocated memory in error paths as well. Please see many other places in the ffmpeg code. Oh, i missed it, i forgot to clean the allocated pkt here, would do in the next version of patch. Thanks for the review Shivam Goyal
From 2aa843626f939218179d3ec252f76f9991c33ed6 Mon Sep 17 00:00:00 2001 From: Shivam Goyal <shivamgoyal1506@outlook.com> Date: Tue, 7 May 2019 10:01:15 +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 | 121 +++++++++++++++++++++++++++++++++++++++ libavformat/version.h | 4 +- 5 files changed, 126 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..55d4938f6d 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,122 @@ static int h264_probe(const AVProbeData *p) return 0; } +static int arecont_h264_probe(const AVProbeData *p) +{ + int i, j, k, o = 0; + int ret = h264_probe(p); + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A}; + + if (!ret) + return 0; + for (i = 0; i + 7 < p->buf_size; i++){ + if (p->buf[i] == id[0] && !memcmp(id, p->buf + i, 8)) + o++; + } + if (o >= 1) + 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, j, k, w = 0; + const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72}; + uint8_t *data; + int64_t pos; + + //Extra to find the http header + size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE; + data = av_malloc(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); + ret = avio_read_partial(s->pb, data, size); + if (ret < 0) { + 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_packet_unref(pkt); + return ret; + } + if (start + RAW_PACKET_SIZE > ret) { + end = ret - 1; + } else + end = start + RAW_PACKET_SIZE - 1; + + //Find and remove http header. + for (i = 0; i < ret; i++) { + if (!(id[0] == data[i] && !memcmp(id, data + i, 6))) + continue; + k = 0; + for (j = i; j + 1 < ret; j++) { + if (!(data[j] == 0x0D && data[j + 1] == 0x0A)) + continue; + k++; + if (k < 4) + continue; + if (i >= start && j + 1 > start && j + 1 <= end) { + memcpy(pkt->data + new_size, data + start, i - start); + new_size += i - start; + memcpy(pkt->data + new_size, data + j + 2, end - j - 1); + new_size += end - j - 1; + w = 1; + break; + } else if (i < start && j + 1 >= start && j + 1 < end) { + memcpy(pkt->data + new_size, data + j + 2, end - j -1); + new_size += end - j - 1; + w = 1; + break; + } else if (j + 1 > end && i > start && i <= end) { + memcpy(pkt->data + new_size, data + start, i - start); + new_size += i - start; + w = 1; + break; + } + } + if (w) + break; + } + if (!w) { + memcpy(pkt->data + new_size, data + start, end - start + 1); + new_size = end - start + 1; + } + avio_seek(s->pb, pos + end - start + 1, SEEK_SET); + av_shrink_packet(pkt, new_size); + 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