| Message ID | 20160927180305.6132-2-jamrial@gmail.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Tue, Sep 27, 2016 at 8:03 PM, James Almer <jamrial@gmail.com> wrote: > It's listed as mandatory in https://matroska.org/technical/specs/index.html > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > The spec also mentions FieldOrder "MUST be ignored if FlagInterlaced is not > set to interlaced". Since it's a mandatory element, i interpreted that as a > demuxer guideline. > Its a mandatory field with a default value - that kind of definition is a bit weird, but the general consensus is that it does not have to be written if it would write the default anyway. - Hendrik
On 9/27/2016 5:56 PM, Hendrik Leppkes wrote: > On Tue, Sep 27, 2016 at 8:03 PM, James Almer <jamrial@gmail.com> wrote: >> It's listed as mandatory in https://matroska.org/technical/specs/index.html >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> The spec also mentions FieldOrder "MUST be ignored if FlagInterlaced is not >> set to interlaced". Since it's a mandatory element, i interpreted that as a >> demuxer guideline. >> > > Its a mandatory field with a default value - that kind of definition > is a bit weird, but the general consensus is that it does not have to > be written if it would write the default anyway. > > - Hendrik I find it weird it was defined this way, considering the precedent set by BlockDuration which is not tagged as mandatory but its description explicitly says "This Element is mandatory when DefaultDuration is set for the track". Alright then, patch dropped from the set.
On Tue, Sep 27, 2016 at 11:14 PM, James Almer <jamrial@gmail.com> wrote: > On 9/27/2016 5:56 PM, Hendrik Leppkes wrote: >> On Tue, Sep 27, 2016 at 8:03 PM, James Almer <jamrial@gmail.com> wrote: >>> It's listed as mandatory in https://matroska.org/technical/specs/index.html >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> The spec also mentions FieldOrder "MUST be ignored if FlagInterlaced is not >>> set to interlaced". Since it's a mandatory element, i interpreted that as a >>> demuxer guideline. >>> >> >> Its a mandatory field with a default value - that kind of definition >> is a bit weird, but the general consensus is that it does not have to >> be written if it would write the default anyway. >> >> - Hendrik > > I find it weird it was defined this way, considering the precedent set by > BlockDuration which is not tagged as mandatory but its description > explicitly says "This Element is mandatory when DefaultDuration is set for > the track". > > Alright then, patch dropped from the set. > For the record, its written here: https://matroska.org/technical/specs/notes.html 4. Mandatory - This element is mandatory in the file. - Mandatory elements with a default value may be left out of the file. In the absence of a mandatory element, the element's default value is used. - A mandatory element is not written if its parent is not in the file. The BlockDuration is a bit weird, but the same concept applies. If DefaultDuration is set, it becomes mandatory, but it still has a default value (ie. the DefaultDuration), so it doesn't have to be written if it matches this value. This whole concept of mandatory with default can be very confusing, but it is what it is. And you conveniently left out the good part "This Element is mandatory when DefaultDuration is set for the track (but can be omitted as other default values).", specifically mentioning this case =) - Hendrik
On 9/27/2016 6:36 PM, Hendrik Leppkes wrote: > On Tue, Sep 27, 2016 at 11:14 PM, James Almer <jamrial@gmail.com> wrote: >> On 9/27/2016 5:56 PM, Hendrik Leppkes wrote: >>> On Tue, Sep 27, 2016 at 8:03 PM, James Almer <jamrial@gmail.com> wrote: >>>> It's listed as mandatory in https://matroska.org/technical/specs/index.html >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> The spec also mentions FieldOrder "MUST be ignored if FlagInterlaced is not >>>> set to interlaced". Since it's a mandatory element, i interpreted that as a >>>> demuxer guideline. >>>> >>> >>> Its a mandatory field with a default value - that kind of definition >>> is a bit weird, but the general consensus is that it does not have to >>> be written if it would write the default anyway. >>> >>> - Hendrik >> >> I find it weird it was defined this way, considering the precedent set by >> BlockDuration which is not tagged as mandatory but its description >> explicitly says "This Element is mandatory when DefaultDuration is set for >> the track". >> >> Alright then, patch dropped from the set. >> > > For the record, its written here: > https://matroska.org/technical/specs/notes.html > > 4. Mandatory > - This element is mandatory in the file. > - Mandatory elements with a default value may be left out of the file. > In the absence of a mandatory element, the element's default value is > used. > - A mandatory element is not written if its parent is not in the file. Ok, that clears things. For that matter, since FlagInterlaced is also mandatory with a default "Undetermined" value, i guess it would make sense to not write it in such scenario. > > The BlockDuration is a bit weird, but the same concept applies. If > DefaultDuration is set, it becomes mandatory, but it still has a > default value (ie. the DefaultDuration), so it doesn't have to be > written if it matches this value. > This whole concept of mandatory with default can be very confusing, > but it is what it is. > > And you conveniently left out the good part "This Element is mandatory > when DefaultDuration is set for the track (but can be omitted as other > default values).", specifically mentioning this case =) Somehow missed that, heh. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 6cb0376..a766b81 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -798,10 +798,16 @@ static void mkv_write_field_order(AVIOContext *pb, int mode, case AV_FIELD_UNKNOWN: put_ebml_uint(pb, MATROSKA_ID_VIDEOFLAGINTERLACED, MATROSKA_VIDEO_INTERLACE_FLAG_UNDETERMINED); + if (mode != MODE_WEBM) + put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, + MATROSKA_VIDEO_FIELDORDER_UNDETERMINED); break; case AV_FIELD_PROGRESSIVE: put_ebml_uint(pb, MATROSKA_ID_VIDEOFLAGINTERLACED, MATROSKA_VIDEO_INTERLACE_FLAG_PROGRESSIVE); + if (mode != MODE_WEBM) + put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, + MATROSKA_VIDEO_FIELDORDER_PROGRESSIVE); break; case AV_FIELD_TT: case AV_FIELD_BB: diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak index 8e4a1e8..fd242b7 100644 --- a/tests/fate/matroska.mak +++ b/tests/fate/matroska.mak @@ -4,6 +4,6 @@ FATE_MATROSKA-$(call DEMMUX, MATROSKA, MATROSKA) += fate-matroska-remux fate-matroska-remux: CMD = md5 -i $(TARGET_SAMPLES)/vp9-test-vectors/vp90-2-2pass-akiyo.webm -color_trc 4 -c:v copy -fflags +bitexact -strict -2 -f matroska fate-matroska-remux: CMP = oneline -fate-matroska-remux: REF = f08b20b90f158a4de5a02a52c25596b9 +fate-matroska-remux: REF = b745381b5a0b3af0e3f2d7d65a18e20f FATE_SAMPLES_AVCONV += $(FATE_MATROSKA-yes) diff --git a/tests/ref/fate/rgb24-mkv b/tests/ref/fate/rgb24-mkv index 69584f5..0efd49d 100644 --- a/tests/ref/fate/rgb24-mkv +++ b/tests/ref/fate/rgb24-mkv @@ -1,5 +1,5 @@ -0d081c8e151a922435830f95000d3c71 *tests/data/fate/rgb24-mkv.matroska -58321 tests/data/fate/rgb24-mkv.matroska +909a07a48b9d3ae8691daa7eb305c40a *tests/data/fate/rgb24-mkv.matroska +58324 tests/data/fate/rgb24-mkv.matroska #tb 0: 1/10 #media_type 0: video #codec_id 0: rawvideo diff --git a/tests/ref/lavf/mkv b/tests/ref/lavf/mkv index 5b0d386..e43bd02 100644 --- a/tests/ref/lavf/mkv +++ b/tests/ref/lavf/mkv @@ -1,6 +1,6 @@ -5b982c8dfbadc71f51b206cbd10b9a71 *./tests/data/lavf/lavf.mkv -472875 ./tests/data/lavf/lavf.mkv +8ec784fcba03cee098140729315d073e *./tests/data/lavf/lavf.mkv +472878 ./tests/data/lavf/lavf.mkv ./tests/data/lavf/lavf.mkv CRC=0xec6c3c68 -b4a295bae8e6cf536563cb840386f3a4 *./tests/data/lavf/lavf.mkv -320551 ./tests/data/lavf/lavf.mkv +0f064370de5762f91cb9777163e89ae0 *./tests/data/lavf/lavf.mkv +320554 ./tests/data/lavf/lavf.mkv ./tests/data/lavf/lavf.mkv CRC=0xec6c3c68 diff --git a/tests/ref/seek/lavf-mkv b/tests/ref/seek/lavf-mkv index bf34766..12b8267 100644 --- a/tests/ref/seek/lavf-mkv +++ b/tests/ref/seek/lavf-mkv @@ -1,48 +1,48 @@ -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 800 size: 208 +ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 803 size: 208 ret: 0 st:-1 flags:0 ts:-1.000000 -ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1016 size: 27837 +ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1019 size: 27837 ret: 0 st:-1 flags:1 ts: 1.894167 -ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292430 size: 27834 +ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292433 size: 27834 ret: 0 st: 0 flags:0 ts: 0.788000 -ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292430 size: 27834 +ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292433 size: 27834 ret: 0 st: 0 flags:1 ts:-0.317000 -ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1016 size: 27837 +ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1019 size: 27837 ret:-1 st: 1 flags:0 ts: 2.577000 ret: 0 st: 1 flags:1 ts: 1.471000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 320271 size: 209 +ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 320274 size: 209 ret: 0 st:-1 flags:0 ts: 0.365002 -ret: 0 st: 0 flags:1 dts: 0.491000 pts: 0.491000 pos: 146983 size: 27925 +ret: 0 st: 0 flags:1 dts: 0.491000 pts: 0.491000 pos: 146986 size: 27925 ret: 0 st:-1 flags:1 ts:-0.740831 -ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1016 size: 27837 +ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1019 size: 27837 ret:-1 st: 0 flags:0 ts: 2.153000 ret: 0 st: 0 flags:1 ts: 1.048000 -ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292430 size: 27834 +ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292433 size: 27834 ret: 0 st: 1 flags:0 ts:-0.058000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 800 size: 208 +ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 803 size: 208 ret: 0 st: 1 flags:1 ts: 2.836000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 320271 size: 209 +ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 320274 size: 209 ret:-1 st:-1 flags:0 ts: 1.730004 ret: 0 st:-1 flags:1 ts: 0.624171 -ret: 0 st: 0 flags:1 dts: 0.491000 pts: 0.491000 pos: 146983 size: 27925 +ret: 0 st: 0 flags:1 dts: 0.491000 pts: 0.491000 pos: 146986 size: 27925 ret: 0 st: 0 flags:0 ts:-0.482000 -ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1016 size: 27837 +ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1019 size: 27837 ret: 0 st: 0 flags:1 ts: 2.413000 -ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292430 size: 27834 +ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292433 size: 27834 ret:-1 st: 1 flags:0 ts: 1.307000 ret: 0 st: 1 flags:1 ts: 0.201000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 800 size: 208 +ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 803 size: 208 ret: 0 st:-1 flags:0 ts:-0.904994 -ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1016 size: 27837 +ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1019 size: 27837 ret: 0 st:-1 flags:1 ts: 1.989173 -ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292430 size: 27834 +ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292433 size: 27834 ret: 0 st: 0 flags:0 ts: 0.883000 -ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292430 size: 27834 +ret: 0 st: 0 flags:1 dts: 0.971000 pts: 0.971000 pos: 292433 size: 27834 ret: 0 st: 0 flags:1 ts:-0.222000 -ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1016 size: 27837 +ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1019 size: 27837 ret:-1 st: 1 flags:0 ts: 2.672000 ret: 0 st: 1 flags:1 ts: 1.566000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 320271 size: 209 +ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 320274 size: 209 ret: 0 st:-1 flags:0 ts: 0.460008 -ret: 0 st: 0 flags:1 dts: 0.491000 pts: 0.491000 pos: 146983 size: 27925 +ret: 0 st: 0 flags:1 dts: 0.491000 pts: 0.491000 pos: 146986 size: 27925 ret: 0 st:-1 flags:1 ts:-0.645825 -ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1016 size: 27837 +ret: 0 st: 0 flags:1 dts: 0.011000 pts: 0.011000 pos: 1019 size: 27837
It's listed as mandatory in https://matroska.org/technical/specs/index.html Signed-off-by: James Almer <jamrial@gmail.com> --- The spec also mentions FieldOrder "MUST be ignored if FlagInterlaced is not set to interlaced". Since it's a mandatory element, i interpreted that as a demuxer guideline. libavformat/matroskaenc.c | 6 ++++++ tests/fate/matroska.mak | 2 +- tests/ref/fate/rgb24-mkv | 4 ++-- tests/ref/lavf/mkv | 8 ++++---- tests/ref/seek/lavf-mkv | 44 ++++++++++++++++++++++---------------------- 5 files changed, 35 insertions(+), 29 deletions(-)