Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On 2015-03-17 16:13, Andreas Cadhalpun wrote:
On 17.03.2015 10:17, tomas.har...@codemill.se wrote:
On 2015-03-14 18:03, Andreas Cadhalpun wrote:
[PATCH 2/2] mxfenc: don't try to write footer without header:
+if (!mxf-header_written ||
+(s-oformat == ff_mxf_opatom_muxer
!mxf-body_partition_offset)) {
+err = AVERROR_UNKNOWN;
+goto end;
+}
+
AVERROR_UNKNOWN?
It's unclear why the header was not written or body_partition_offset
not allocated. It could e.g. be due to invalid options, not supported
codecs, or just out of memory.
Do you think AVERROR(EINVAL) or even just -1 would be better?
Best regards,
Andreas
No preference really. Perhaps a comment though?
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On Wed, Mar 18, 2015 at 10:01:36PM +0100, Andreas Cadhalpun wrote:
On 18.03.2015 16:59, tomas.har...@codemill.se wrote:
On 2015-03-17 16:13, Andreas Cadhalpun wrote:
On 17.03.2015 10:17, tomas.har...@codemill.se wrote:
On 2015-03-14 18:03, Andreas Cadhalpun wrote:
[PATCH 2/2] mxfenc: don't try to write footer without header:
+if (!mxf-header_written ||
+(s-oformat == ff_mxf_opatom_muxer
!mxf-body_partition_offset)) {
+err = AVERROR_UNKNOWN;
+goto end;
+}
+
AVERROR_UNKNOWN?
It's unclear why the header was not written or body_partition_offset
not allocated. It could e.g. be due to invalid options, not supported
codecs, or just out of memory.
Do you think AVERROR(EINVAL) or even just -1 would be better?
Best regards,
Andreas
No preference really. Perhaps a comment though?
OK. New patch with comment attached.
Best regards,
Andreas
mxfenc.c |7 +++
1 file changed, 7 insertions(+)
db5cf2fdfe8a815e8785669644690907a9a5a7fe
0001-mxfenc-don-t-try-to-write-footer-without-header.patch
From 1424c87b9a786555b294fb9daa9fd2a67be9a30d Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun andreas.cadhal...@googlemail.com
Date: Wed, 18 Mar 2015 21:57:58 +0100
Subject: [PATCH] mxfenc: don't try to write footer without header
This fixes a crash, when trying to mux h264 into mxf_opatom.
applied
thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No great genius has ever existed without some touch of madness. -- Aristotle
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On 17.03.2015 10:17, tomas.har...@codemill.se wrote:
On 2015-03-14 18:03, Andreas Cadhalpun wrote:
[PATCH 2/2] mxfenc: don't try to write footer without header:
+if (!mxf-header_written ||
+(s-oformat == ff_mxf_opatom_muxer
!mxf-body_partition_offset)) {
+err = AVERROR_UNKNOWN;
+goto end;
+}
+
AVERROR_UNKNOWN?
It's unclear why the header was not written or body_partition_offset
not allocated. It could e.g. be due to invalid options, not supported
codecs, or just out of memory.
Do you think AVERROR(EINVAL) or even just -1 would be better?
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On Tue, Mar 17, 2015 at 10:17:06AM +0100, tomas.har...@codemill.se wrote: On 2015-03-14 18:03, Andreas Cadhalpun wrote: On 14.03.2015 02:17, Mark Reid wrote: On Fri, Mar 13, 2015 at 6:02 AM, Andreas Cadhalpun andreas.cadhal...@googlemail.com wrote: [...] Memleak patch is obviously OK. applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB DNS cache poisoning attacks, popular search engine, Google internet authority dont be evil, please signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On 14.03.2015 02:17, Mark Reid wrote:
On Fri, Mar 13, 2015 at 6:02 AM, Andreas Cadhalpun
andreas.cadhal...@googlemail.com wrote:
On 13.03.2015 11:59, Tomas Härdin wrote:
A better solution would
be to figure out why mxf-body_partition_offset becomes NULL so that
index tables and such can be rewritten properly.
It can always happen that mxf-body_partition_offset is NULL, e.g. if
no memory is left, or if something else fails. Try e.g.:
ffmpeg -f lavfi -i testsrc -c:v libx264 -f mxf_opatom
mxf-body_partition_offset is NULL because currently only AVC Intra 50/100
h264 is supported.
Yes.
The encoder figures out the h264 format by parsing the
h264 packet and doesn't write the body partiton (or even the header
partition) untill after it parses the first packet. If the packet is
invalid, nothing get written and mxf-body_partition_offset doesn't get
allocated.
That's correct.
perhaps mxf_write_footer should return a error if
mxf-body_partition_offset is NULL or just if mxf-header_written == 0
before doing trying to write anything.
Well, mxf_write_footer also has to free some allocated memory, which would
get leaked if one just returns...
...like it does in any of the currently present 'return err' cases.
Attached is a patch fixing the memleaks and another returning an error,
if no header was written before the footer.
Best regards,
Andreas
From ac6970e62ddd65fe18be10f1c79f10c6d8ce3a75 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun andreas.cadhal...@googlemail.com
Date: Sat, 14 Mar 2015 17:48:56 +0100
Subject: [PATCH 2/2] mxfenc: don't try to write footer without header
This fixes a crash, when trying to mux h264 into mxf_opatom.
Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
---
libavformat/mxfenc.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index bf680f8..2485741 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2340,6 +2340,12 @@ static int mxf_write_footer(AVFormatContext *s)
AVIOContext *pb = s-pb;
int err = 0;
+if (!mxf-header_written ||
+(s-oformat == ff_mxf_opatom_muxer !mxf-body_partition_offset)) {
+err = AVERROR_UNKNOWN;
+goto end;
+}
+
mxf-duration = mxf-last_indexed_edit_unit + mxf-edit_units_count;
mxf_write_klv_fill(s);
--
2.1.4
From 5d9c6182f3f203fcbb286012d13cff76f9083b57 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun andreas.cadhal...@googlemail.com
Date: Sat, 14 Mar 2015 17:47:53 +0100
Subject: [PATCH 1/2] mxfenc: fix memleaks in mxf_write_footer
Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
---
libavformat/mxfenc.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 898951c..bf680f8 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2338,7 +2338,7 @@ static int mxf_write_footer(AVFormatContext *s)
{
MXFContext *mxf = s-priv_data;
AVIOContext *pb = s-pb;
-int err;
+int err = 0;
mxf-duration = mxf-last_indexed_edit_unit + mxf-edit_units_count;
@@ -2346,10 +2346,10 @@ static int mxf_write_footer(AVFormatContext *s)
mxf-footer_partition_offset = avio_tell(pb);
if (mxf-edit_unit_byte_count s-oformat != ff_mxf_opatom_muxer) { // no need to repeat index
if ((err = mxf_write_partition(s, 0, 0, footer_partition_key, 0)) 0)
-return err;
+goto end;
} else {
if ((err = mxf_write_partition(s, 0, 2, footer_partition_key, 0)) 0)
-return err;
+goto end;
mxf_write_klv_fill(s);
mxf_write_index_table_segment(s);
}
@@ -2362,21 +2362,22 @@ static int mxf_write_footer(AVFormatContext *s)
/* rewrite body partition to update lengths */
avio_seek(pb, mxf-body_partition_offset[0], SEEK_SET);
if ((err = mxf_write_opatom_body_partition(s)) 0)
-return err;
+goto end;
}
avio_seek(pb, 0, SEEK_SET);
if (mxf-edit_unit_byte_count s-oformat != ff_mxf_opatom_muxer) {
if ((err = mxf_write_partition(s, 1, 2, header_closed_partition_key, 1)) 0)
-return err;
+goto end;
mxf_write_klv_fill(s);
mxf_write_index_table_segment(s);
} else {
if ((err = mxf_write_partition(s, 0, 0, header_closed_partition_key, 1)) 0)
-return err;
+goto end;
}
}
+end:
ff_audio_interleave_close(s);
av_freep(mxf-index_entries);
@@ -2386,7 +2387,7 @@ static int mxf_write_footer(AVFormatContext *s)
mxf_free(s);
-return 0;
+return err 0 ? err : 0;
}
static int mxf_interleave_get_packet(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush)
--
2.1.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On Fri, Mar 13, 2015 at 6:02 AM, Andreas Cadhalpun
andreas.cadhal...@googlemail.com wrote:
On 13.03.2015 11:59, Tomas Härdin wrote:
On Thu, 2015-03-12 at 17:48 +0100, Andreas Cadhalpun wrote:
This fixes a crash, when trying to mux h264 into mxf_opatom.
Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
---
libavformat/mxfenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 898951c..2891f5d 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2358,7 +2358,7 @@ static int mxf_write_footer(AVFormatContext *s)
mxf_write_random_index_pack(s);
if (s-pb-seekable) {
-if (s-oformat == ff_mxf_opatom_muxer){
+if (s-oformat == ff_mxf_opatom_muxer
mxf-body_partition_offset){
/* rewrite body partition to update lengths */
avio_seek(pb, mxf-body_partition_offset[0], SEEK_SET);
if ((err = mxf_write_opatom_body_partition(s)) 0)
Doesn't this need to happen for H.264 as well?
Maybe, but the seek can't work if mxf-body_partition_offset is NULL.
Would it be better to add the check only around the seek?
A better solution would
be to figure out why mxf-body_partition_offset becomes NULL so that
index tables and such can be rewritten properly.
It can always happen that mxf-body_partition_offset is NULL, e.g. if
no memory is left, or if something else fails. Try e.g.:
ffmpeg -f lavfi -i testsrc -c:v libx264 -f mxf_opatom
mxf-body_partition_offset is NULL because currently only AVC Intra 50/100
h264 is supported. The encoder figures out the h264 format by parsing the
h264 packet and doesn't write the body partiton (or even the header
partition) untill after it parses the first packet. If the packet is
invalid, nothing get written and mxf-body_partition_offset doesn't get
allocated. perhaps mxf_write_footer should return a error if
mxf-body_partition_offset is NULL or just if mxf-header_written == 0
before doing trying to write anything.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On Thu, 2015-03-12 at 17:48 +0100, Andreas Cadhalpun wrote:
This fixes a crash, when trying to mux h264 into mxf_opatom.
Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
---
libavformat/mxfenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 898951c..2891f5d 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2358,7 +2358,7 @@ static int mxf_write_footer(AVFormatContext *s)
mxf_write_random_index_pack(s);
if (s-pb-seekable) {
-if (s-oformat == ff_mxf_opatom_muxer){
+if (s-oformat == ff_mxf_opatom_muxer
mxf-body_partition_offset){
/* rewrite body partition to update lengths */
avio_seek(pb, mxf-body_partition_offset[0], SEEK_SET);
if ((err = mxf_write_opatom_body_partition(s)) 0)
Doesn't this need to happen for H.264 as well? A better solution would
be to figure out why mxf-body_partition_offset becomes NULL so that
index tables and such can be rewritten properly.
/Tomas
signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it
On 13.03.2015 11:59, Tomas Härdin wrote:
On Thu, 2015-03-12 at 17:48 +0100, Andreas Cadhalpun wrote:
This fixes a crash, when trying to mux h264 into mxf_opatom.
Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
---
libavformat/mxfenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 898951c..2891f5d 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2358,7 +2358,7 @@ static int mxf_write_footer(AVFormatContext *s)
mxf_write_random_index_pack(s);
if (s-pb-seekable) {
-if (s-oformat == ff_mxf_opatom_muxer){
+if (s-oformat == ff_mxf_opatom_muxer
mxf-body_partition_offset){
/* rewrite body partition to update lengths */
avio_seek(pb, mxf-body_partition_offset[0], SEEK_SET);
if ((err = mxf_write_opatom_body_partition(s)) 0)
Doesn't this need to happen for H.264 as well?
Maybe, but the seek can't work if mxf-body_partition_offset is NULL.
Would it be better to add the check only around the seek?
A better solution would
be to figure out why mxf-body_partition_offset becomes NULL so that
index tables and such can be rewritten properly.
It can always happen that mxf-body_partition_offset is NULL, e.g. if
no memory is left, or if something else fails. Try e.g.:
ffmpeg -f lavfi -i testsrc -c:v libx264 -f mxf_opatom
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
