| Message ID | 1511365263-17845-1-git-send-email-t.rapp@noa-archive.com |
|---|---|
| State | New |
| Headers | show |
On 11/22/2017 3:41 PM, Tobias Rapp wrote: > Writes one set of field framing information for progressive streams and > two sets for interlaced streams. Fixes ticket #6383. > > Unfortunately the OpenDML v1.02 document is not very specific what value > to use for start_line when frame data is not coming from a capturing > device, so this is just using 0/1 depending on the field order as a > best-effort guess. Looking at the OpenDML spec, I think this indeed is the best we can do without deeper knowledge of where the source signal came from, or copying it from a pre-existing file, within the existing avformat scaffolding. Code itself seems OK. - Derek
2017-11-22 16:41 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>: > Writes one set of field framing information for progressive streams and > two sets for interlaced streams. Fixes ticket #6383. > > Unfortunately the OpenDML v1.02 document is not very specific what value > to use for start_line when frame data is not coming from a capturing > device, so this is just using 0/1 depending on the field order as a > best-effort guess. I believe your approach is sane but the only available examples may indicate that it should be set to something like height / 2 ;-( > + int num, den, fields, i; > av_reduce(&num, &den, dar.num, dar.den, 0xFFFF); > + if (par->field_order == AV_FIELD_TT || par->field_order == AV_FIELD_BB || > + par->field_order == AV_FIELD_TB || par->field_order == AV_FIELD_BT) { > + fields = 2; // interlaced > + } else { > + fields = 1; // progressive fields = 1 + field_order == TT || field_order == BB etc.; > + for (i = 0; i < fields; i++) { > + int start_line; > + if (par->field_order == AV_FIELD_TT || par->field_order == AV_FIELD_TB) { > + start_line = (i == 0) ? 0 : 1; > + } else if (par->field_order == AV_FIELD_BB || par->field_order == AV_FIELD_BT) { > + start_line = (i == 0) ? 1 : 0; start_line = fields * (i ^ (par->field_order == AV_FIELD_BB || par->field_order == AV_FIELD_BT)); Which are imo less ugly. Carl Eugen
On 11/22/2017 10:52 PM, Carl Eugen Hoyos wrote: > start_line = fields * (i ^ (par->field_order == AV_FIELD_BB || > par->field_order == AV_FIELD_BT)); > > Which are imo less ugly. Can't agree. It's needlessly less readable bit fiddling. - Derek
On 22.11.2017 23:52, Carl Eugen Hoyos wrote: > 2017-11-22 16:41 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>: >> Writes one set of field framing information for progressive streams and >> two sets for interlaced streams. Fixes ticket #6383. >> >> Unfortunately the OpenDML v1.02 document is not very specific what value >> to use for start_line when frame data is not coming from a capturing >> device, so this is just using 0/1 depending on the field order as a >> best-effort guess. > > I believe your approach is sane but the only available examples > may indicate that it should be set to something like height / 2 ;-( Indeed my main problem is that I have not found some real-world example file with two sets of field framing information. I scanned the files at http://streams.videolan.org/samples/ but out of >900 AVI files only about 40 files contain a vprp chunk but none contains two fields. I have found two files that have FieldPerFrame=2 (indicating an interlaced video stream) but they contain a truncated vprp chunk (no field framing information): http://streams.videolan.org/samples/V-codecs/MJPEGs/matrox-capture.avi http://streams.videolan.org/samples/avi/TRA3106.avi As this clearly doesn't match the specs I didn't want to follow these two examples. I can use height/2 for start_line offset instead of 1 if that sounds better, to me it would look like an indication that the fields are stored separated, but no strong opinion from my side. Regards, Tobias
2017-11-23 9:40 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>: > On 22.11.2017 23:52, Carl Eugen Hoyos wrote: >> >> 2017-11-22 16:41 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>: >>> >>> Writes one set of field framing information for progressive streams and >>> two sets for interlaced streams. Fixes ticket #6383. >>> >>> Unfortunately the OpenDML v1.02 document is not very specific what value >>> to use for start_line when frame data is not coming from a capturing >>> device, so this is just using 0/1 depending on the field order as a >>> best-effort guess. >> >> >> I believe your approach is sane but the only available examples >> may indicate that it should be set to something like height / 2 ;-( > I can use height/2 for start_line offset instead of 1 I tried hard not to imply that I would prefer this. Carl Eugen
On 23.11.2017 00:10, Derek Buitenhuis wrote: > On 11/22/2017 10:52 PM, Carl Eugen Hoyos wrote: >> start_line = fields * (i ^ (par->field_order == AV_FIELD_BB || >> par->field_order == AV_FIELD_BT)); >> >> Which are imo less ugly. > > Can't agree. > > It's needlessly less readable bit fiddling. +1 It may save some lines by in my opinion the current code is more readable and maintainable in case there are changes to AVFieldOrder (see discussion at https://ffmpeg.org/pipermail/ffmpeg-devel/2017-September/215698.html). Regards, Tobias
On 22.11.2017 16:58, Derek Buitenhuis wrote: > On 11/22/2017 3:41 PM, Tobias Rapp wrote: >> Writes one set of field framing information for progressive streams and >> two sets for interlaced streams. Fixes ticket #6383. >> >> Unfortunately the OpenDML v1.02 document is not very specific what value >> to use for start_line when frame data is not coming from a capturing >> device, so this is just using 0/1 depending on the field order as a >> best-effort guess. > > Looking at the OpenDML spec, I think this indeed is the best we can do without > deeper knowledge of where the source signal came from, or copying it from a > pre-existing file, within the existing avformat scaffolding. > > Code itself seems OK. Copied part of the commit message as a comment to the code and pushed. Thanks Carl and Derek for the review. Regards, Tobias
diff --git a/libavformat/avienc.c b/libavformat/avienc.c index 483f5b5..efe67ce 100644 --- a/libavformat/avienc.c +++ b/libavformat/avienc.c @@ -501,8 +501,14 @@ static int avi_write_header(AVFormatContext *s) AVRational dar = av_mul_q(st->sample_aspect_ratio, (AVRational) { par->width, par->height }); - int num, den; + int num, den, fields, i; av_reduce(&num, &den, dar.num, dar.den, 0xFFFF); + if (par->field_order == AV_FIELD_TT || par->field_order == AV_FIELD_BB || + par->field_order == AV_FIELD_TB || par->field_order == AV_FIELD_BT) { + fields = 2; // interlaced + } else { + fields = 1; // progressive + } avio_wl32(pb, 0); // video format = unknown avio_wl32(pb, 0); // video standard = unknown @@ -514,17 +520,27 @@ static int avi_write_header(AVFormatContext *s) avio_wl16(pb, num); avio_wl32(pb, par->width); avio_wl32(pb, par->height); - avio_wl32(pb, 1); // progressive FIXME - - avio_wl32(pb, par->height); - avio_wl32(pb, par->width); - avio_wl32(pb, par->height); - avio_wl32(pb, par->width); - avio_wl32(pb, 0); - avio_wl32(pb, 0); + avio_wl32(pb, fields); // fields per frame + + for (i = 0; i < fields; i++) { + int start_line; + if (par->field_order == AV_FIELD_TT || par->field_order == AV_FIELD_TB) { + start_line = (i == 0) ? 0 : 1; + } else if (par->field_order == AV_FIELD_BB || par->field_order == AV_FIELD_BT) { + start_line = (i == 0) ? 1 : 0; + } else { + start_line = 0; + } - avio_wl32(pb, 0); - avio_wl32(pb, 0); + avio_wl32(pb, par->height / fields); // compressed bitmap height + avio_wl32(pb, par->width); // compressed bitmap width + avio_wl32(pb, par->height / fields); // valid bitmap height + avio_wl32(pb, par->width); // valid bitmap width + avio_wl32(pb, 0); // valid bitmap X offset + avio_wl32(pb, 0); // valid bitmap Y offset + avio_wl32(pb, 0); // valid X offset in T + avio_wl32(pb, start_line); // valid Y start line + } ff_end_tag(pb, vprp); } diff --git a/libavformat/version.h b/libavformat/version.h index feb1461..7fe3710 100644 --- a/libavformat/version.h +++ b/libavformat/version.h @@ -33,7 +33,7 @@ // Also please add any ticket numbers that you believe might be affected here #define LIBAVFORMAT_VERSION_MAJOR 58 #define LIBAVFORMAT_VERSION_MINOR 2 -#define LIBAVFORMAT_VERSION_MICRO 102 +#define LIBAVFORMAT_VERSION_MICRO 103 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \ LIBAVFORMAT_VERSION_MINOR, \ diff --git a/tests/ref/fate/copy-trac2211-avi b/tests/ref/fate/copy-trac2211-avi index 007349e..06d81e5 100644 --- a/tests/ref/fate/copy-trac2211-avi +++ b/tests/ref/fate/copy-trac2211-avi @@ -1,5 +1,5 @@ -6f6b211cbc8de9871e8e09e64048e2f9 *tests/data/fate/copy-trac2211-avi.avi -1777924 tests/data/fate/copy-trac2211-avi.avi +0920978f3f8196413c43f0033b55a5b6 *tests/data/fate/copy-trac2211-avi.avi +1777956 tests/data/fate/copy-trac2211-avi.avi #tb 0: 1/14 #media_type 0: video #codec_id 0: rawvideo
Writes one set of field framing information for progressive streams and two sets for interlaced streams. Fixes ticket #6383. Unfortunately the OpenDML v1.02 document is not very specific what value to use for start_line when frame data is not coming from a capturing device, so this is just using 0/1 depending on the field order as a best-effort guess. Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com> --- libavformat/avienc.c | 38 +++++++++++++++++++++++++++----------- libavformat/version.h | 2 +- tests/ref/fate/copy-trac2211-avi | 4 ++-- 3 files changed, 30 insertions(+), 14 deletions(-)