Re: [FFmpeg-devel] [PATCH v23 04/10] avformat/evc_demuxer: Added demuxer to handle reading EVC video files

["Dawid Kozinski/Multimedia \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics" <d.kozinski@samsung.com>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James
> Almer
> Sent: środa, 7 czerwca 2023 18:38
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v23 04/10] avformat/evc_demuxer: Added
> demuxer to handle reading EVC video files
> 
> On 6/7/2023 1:30 PM, Dawid Kozinski/Multimedia (PLT) /SRPOL/Staff
> Engineer/Samsung Electronics wrote:
> >
> >
> >
> >
> >> -----Original Message-----
> >> From: Dawid Kozinski/Multimedia (PLT) /SRPOL/Staff Engineer/Samsung
> >> Electronics <d.kozinski@samsung.com>
> >> Sent: środa, 7 czerwca 2023 17:41
> >> To: 'FFmpeg development discussions and patches' <ffmpeg-
> >> devel@ffmpeg.org>
> >> Subject: RE: [FFmpeg-devel] [PATCH v23 04/10] avformat/evc_demuxer:
> >> Added demuxer to handle reading EVC video files
> >>
> >>
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >>> James Almer
> >>> Sent: środa, 7 czerwca 2023 15:40
> >>> To: ffmpeg-devel@ffmpeg.org
> >>> Subject: Re: [FFmpeg-devel] [PATCH v23 04/10] avformat/evc_demuxer:
> >>> Added demuxer to handle reading EVC video files
> >>>
> >>> On 6/7/2023 10:36 AM, Dawid Kozinski/Multimedia (PLT) /SRPOL/Staff
> >>> Engineer/Samsung Electronics wrote:
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >>>>> James Almer
> >>>>> Sent: poniedziałek, 29 maja 2023 15:08
> >>>>> To: ffmpeg-devel@ffmpeg.org
> >>>>> Subject: Re: [FFmpeg-devel] [PATCH v23 04/10] avformat/evc_demuxer:
> >>>>> Added demuxer to handle reading EVC video files
> >>>>>
> >>>>> On 5/26/2023 7:31 AM, Dawid Kozinski wrote:
> >>>>>> +static int get_temporal_id(const uint8_t *bits, int bits_size) {
> >>>>>> +    int temporal_id = 0;
> >>>>>> +    short t = 0;
> >>>>>> +
> >>>>>> +    if (bits_size >= EVC_NALU_HEADER_SIZE) {
> >>>>>> +        unsigned char *p = (unsigned char *)bits;
> >>>>>> +        // forbidden_zero_bit
> >>>>>> +        if ((p[0] & 0x80) != 0)
> >>>>>> +            return -1;
> >>>>>> +
> >>>>>> +        for (int i = 0; i < EVC_NALU_HEADER_SIZE; i++)
> >>>>>> +            t = (t << 8) | p[i];
> >>>>>
> >>>>> I think this can be replaced with AV_RB16.
> >>>>>
> >>>>>> +
> >>>>>> +        temporal_id = (t >> 6) & 0x0007;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    return temporal_id;
> >>>>>> +}
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>> +static uint32_t read_nal_unit_length(const uint8_t *bits, int
> >>>>>> +bits_size) {
> >>>>>> +    uint32_t nalu_len = 0;
> >>>>>> +
> >>>>>> +    if (bits_size >= EVC_NALU_LENGTH_PREFIX_SIZE) {
> >>>>>> +
> >>>>>> +        int t = 0;
> >>>>>> +        unsigned char *p = (unsigned char *)bits;
> >>>>>> +
> >>>>>> +        for (int i = 0; i < EVC_NALU_LENGTH_PREFIX_SIZE; i++)
> >>>>>> +            t = (t << 8) | p[i];
> >>>>>
> >>>>> AV_RB32.
> >>>>>
> >>>>>> +
> >>>>>> +        nalu_len = t;
> >>>>>> +        if (nalu_len == 0)   // Invalid bitstream size
> >>>>>> +            return 0;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    return nalu_len;
> >>>>>> +}
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>> +static int evc_frame_merge_filter(AVBSFContext *bsf, AVPacket *out) {
> >>>>>> +    EVCMergeContext *ctx = bsf->priv_data;
> >>>>>> +    AVPacket *in = ctx->in;
> >>>>>> +
> >>>>>> +    int free_space = 0;
> >>>>>> +    size_t  nalu_size = 0;
> >>>>>> +    uint8_t *nalu = NULL;
> >>>>>> +    int au_end_found = 0;
> >>>>>> +    int err;
> >>>>>> +
> >>>>>> +    err = ff_bsf_get_packet_ref(bsf, in);
> >>>>>> +    if (err < 0)
> >>>>>> +        return err;
> >>>>>> +
> >>>>>> +    nalu_size = read_nal_unit_length(in->data,
> >>>>> EVC_NALU_LENGTH_PREFIX_SIZE);
> >>>>>> +    if(nalu_size <= 0) {
> >>>>>> +        av_packet_unref(in);
> >>>>>> +        return AVERROR_INVALIDDATA;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    nalu = in->data + EVC_NALU_LENGTH_PREFIX_SIZE;
> >>>>>> +    nalu_size = in->size - EVC_NALU_LENGTH_PREFIX_SIZE;
> >>>>>> +
> >>>>>> +    // NAL unit parsing needed to determine if end of AU was found
> >>>>>> +    err = parse_nal_unit(nalu, nalu_size, bsf);
> >>>>>> +    if (err < 0) {
> >>>>>> +        av_log(bsf, AV_LOG_ERROR, "NAL Unit parsing error\n");
> >>>>>> +        av_packet_unref(in);
> >>>>>> +
> >>>>>> +        return err;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    au_end_found = end_of_access_unit_found(bsf);
> >>>>>> +
> >>>>>> +    free_space = ctx->au_buffer.capacity - ctx->au_buffer.data_size;
> >>>>>> +    while( free_space < in->size ) {
> >>>>>> +        ctx->au_buffer.capacity *= 2;
> >>>>>> +        free_space = ctx->au_buffer.capacity -
> >>>>>> + ctx->au_buffer.data_size;
> >>>>>> +
> >>>>>> +        if(free_space >= in->size) {
> >>>>>> +            ctx->au_buffer.data =
> >>>>>> + av_realloc(ctx->au_buffer.data,
> >>>>>> + ctx-
> >>>>>> au_buffer.capacity);
> >>>>>> +        }
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    memcpy(ctx->au_buffer.data + ctx->au_buffer.data_size,
> >>>>>> + in->data,
> >>>>>> + in->size);
> >>>>>
> >>>>> This is including the Annex-B's nal_unit_lenght value into the
> >>>>> assembled
> >>>> packet. I
> >>>>> assume libxevd expects this? Also, is it defined as part of the
> >>>>> ISOBMFF encapsulation? Patch 07/10 does not seem to strip it.
> >>>>
> >>>> Could you expand on this topic?
> >>>> What do you mean by "Patch 07/10 does not seem to strip it"?
> >>>> Patch 7/10 expands the implementation of the MOV Muxer to support
> >>>> the EVC stream.
> >>>> The function ff_isom_write_evcc parses the received data, extracts
> >>>> NAL units of types SPS, PPS, APS, SEI, and inserts their contents
> >>>> into the corresponding elements of the arrays array, which is a
> >>>> component of the EVCDecoderConfigurationRecord structure.
> >>>> Additionally, the ff_isom_write_evcc function populates the
> >>>> relevant fields of the EVCDecoderConfigurationRecord structure
> >>>> based on the parsed SPS NAL unit data, and then calls the
> >>>> evcc_write function, which writes the EVCDecoderConfigurationRecord to
> the stream.
> >>>>
> >>>> It seems that I don't understand what's wrong with our
> >>>> implementation ( I mean MOV muxer for EVC) and it seems I need your
> >>>> help in this matter. I need an explanation of what's wrong and what
> >>>> should be done to fix
> >>> the issue.
> >>>
> >>> I mean the packets that go into ISOBMFF Sample, not the evcc atom.
> >>> Is the encapsulation supposed to include the nal_unit_length from Annex-B?
> >>
> >> The function mov_write_single_packet(s, pkt) receives packets
> >> containing the Access Units (AU) of an EVC stream. Each AU consists
> >> of a sequence of NAL units, each preceded by a 4-byte prefix that
> >> contains information about the length of the NAL unit. The entire AU
> >> is then copied to AVIOContext using avio_write(pb, pkt->data, size).
> >>
> >
> > It seems to me that everything is okay. If I am mistaken, just let me know.
> > As we have also completed all the fixes following your review this is the last
> thing blocking us from creating a new patchset.
> 
> If the 4 byte prefix goes into ISOBMFF encapsulation then, it's fine.

Yes it does. 4 byte prefix goes into ISOBMFF encapsulation.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://protect2.fireeye.com/v1/url?k=f5de603c-94a5caab-f5dfeb73-
> 74fe4860001d-9b05bfc043336197&q=1&e=f929075b-a156-4c1b-a348-
> cb29aff68ccd&u=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmpe
> g-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".