Bmx Merge Request #1: This is our edit for bmx (open)

Last commits (yuyv & mpeg 2 pal) require our libmxf.

Warning: some fields are filled with the garbage:

MPEG2LGMXFDescriptorhelper : 277

        case MPEG2_422P_ML_IFrame:
        case MPEG2_422P_ML:
        case MPEG2_MP_ML:
            cdci_descriptor->setSampledWidth(720);
            cdci_descriptor->setSampledHeight(576);
            cdci_descriptor->setStoredWidth(720);
            cdci_descriptor->setStoredHeight(576);
            cdci_descriptor->setDisplayWidth(720);
            cdci_descriptor->setDisplayHeight(576);
            cdci_descriptor->appendVideoLineMap(0);
            cdci_descriptor->appendVideoLineMap(0);
            break;

[Philip de Nier]

Thanks for this. I'll comment on each commit using the commit id. A general comment is that it is difficult for me to verify the files are correct because there is no way for me to create them using either raw2bmx or bmxtranswrap. A regression test can't be added either to ensure these formats don't get broken by future development work.

49c56a:
I'm not sure this is a bug. I can see that calling CompleteWrite() without first calling PrepareWrite() will cause a segmentation fault but I would think that the CompleteWrite() shouldn't have been called

40ba1d:
What your commit is doing is removing the safeguard rather than adding one, which appears to contradict what your commit message is saying. I can't remember the exact details about pic_order_cnt and will have to take a another look at the H264 spec. at some point. I'm not sure whether allowing the value to be overwritten in mDecodeFrames is what the code logic would support properly in all cases, which was probably why I added the check.

1d64c3:
See my comments in libMXF regarding the name. In the SUPPORTED_ESSENCE table I would prefer to place YUYV... after the UNC... in the table. The Avid avid_resolution_id should be set to 0x00 (unknown). An entry for 30000/1001 needs to be added if that rate is to be supported.

b11da2:
See my comments in libMXF regarding the invalid MPEG_MP_ML and MPEG2_422P_ML labels. The descriptor properties related to these formats appear to be incorrect, e.g. there are HD descriptor values associated with these format when these formats are SD, videolinemap should be set etc. I note that you acknowldge this in the merge request above: "Warning: some fields are filled with the garbage", but this would need to be fixed for it to be acceptable. Also, it properties shouldn't be limited to PAL.

[Aleksandr.Slobodeniuk]

49c56a:

Case of "CompleteWrite() before PrepareWrite()" can happen due to emergency completion.

For example: we created a file, but something went wrong during the creation of the streams.

[Philip de Nier]

49c56a:

CompleteWrite must only be called after PrepareWrite. CompleteWrite finishes what PrepareWrite started.

[Aleksandr.Slobodeniuk]

please, could you write down the standard you have used to fill this fields:

    cdci_descriptor->setSampledWidth(1280);
    cdci_descriptor->setSampledHeight(720);
    cdci_descriptor->setStoredWidth(1280);
    cdci_descriptor->setStoredHeight(720);
    cdci_descriptor->setDisplayWidth(1280);
    cdci_descriptor->setDisplayHeight(720);
    cdci_descriptor->appendVideoLineMap(26);
    cdci_descriptor->appendVideoLineMap(0);

I couldn't find it, that's why I made setStoredHeight() filled with the same value as setSampledHeight() and appendVideoLineMap() filled with 0.

[Philip de Nier]

The MXF specification defines those properties. Have a look at the table in UncCDCIMXFDescriptorHelper.cpp which includes video line map values for SD. So, for 625-line SD I would expect to see the Sampled, Stored and Display dimensions set to 720x288 (height is field height because the frame layout is Seperate Fields) and the video line map for the 2 fields set to {23, 336}. A good way to confirm descriptor values is to compare them with MXF files produced by well known manufacturers.

Philip

[Aleksandr.Slobodeniuk]

Commit 40ba1d is really wrong. It lets BMX set wrong key frame offset. Sorry for wasting your time. I'm going to fix this problem soon (this/next month).

Other commits are corrected to some point.
For now I can't find the time to support executable projects like bmxtranswrap.

[Aleksandr.Slobodeniuk]

ok, explaining the problem, faced with commit 40ba1d.

Imagine, you have an AVC stream, with one field in one nal-unit. And you want to create MXF with one frame in one essence container edit unit. This means two fields (two nal-units with slices) in single essence edit unit.

And the stream has first field as I-frame, and second field as P-frame. Other frames have the same fields (P-P , B-B).

So, what key_frame_offset and temporal_offset are we going to write into the index table?
The problem of bmx's code in this case is that it gives priority to second slice:

AVCEssenceParser.cpp : 621

            if (slice_header.redundant_pic_cnt == 0) {
                MPEGFrameType slice_frame_type = UNKNOWN_FRAME_TYPE;
                if (slice_header.slice_type > 9)
                    BMX_EXCEPTION(("Unknown slice_type %" PRIu64, slice_header.slice_type));
                if (IS_I(slice_header.slice_type) || IS_SI(slice_header.slice_type))
                    slice_frame_type = I_FRAME;
                else if (IS_P(slice_header.slice_type) || IS_SP(slice_header.slice_type))
                    slice_frame_type = P_FRAME;
                else
                    slice_frame_type = B_FRAME;

                if (mFrameType == UNKNOWN_FRAME_TYPE) {
                    mFrameType  = slice_frame_type;
                    mIsIDRFrame = slice_header.idr_pic_flag;
                } else if (mFrameType == I_FRAME) {
                    if (slice_frame_type != I_FRAME)
                        mFrameType = slice_frame_type;
                } else if (mFrameType == P_FRAME) {
                    if (slice_frame_type == B_FRAME)
                        mFrameType = slice_frame_type;
                }

                if (mFrameType != I_FRAME || !slice_header.idr_pic_flag)
                    mIsIDRFrame = false;

So, the code works like it never met I-frame or Idr, re-writing mFrameType and mIsIDRFrame in the priority of the second slice.

My new solution is that I've changed the priority to I frame and Idr. This means if we met Idr nal-unit, in data for essence edit unit, we set mIsIDRFrame = true, and if we met I-frame, we force mFrameType = true.

Looks like it works ok, but I'm not sure yet, expecially about temporal offsets , so I'll send you a commit little bit later, after some testing.

[Philip de Nier]

OK, I can see why there could be an issue here. Support for field encoded H264 has not yet been implemented (#3 in TODO-AVC.txt). I can't remember exactly what was needed. The changes would have to combine fields (either in the parser or writer helper - probably the latter) and deal with your case where a frame contains different H264 frame types. I guess in the case of an I-frame field followed by P-frame field that the MXF frame is marked as a keyframe and temporal offser 0 in the MXF index table.

Philip