Re: [PATCH 01/31] staging: bcm2835-camera: Ensure H264 header bytes get a sensible timestamp

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 6/28/19 4:00 PM, Nicolas Dufresne wrote:
> Le vendredi 28 juin 2019 à 11:10 +0100, Dave Stevenson a écrit :
>> Hi Nicolas
>>
>> On Thu, 27 Jun 2019 at 20:55, Nicolas Dufresne <nicolas@xxxxxxxxxxxx> wrote:
>>> Hi Dave,
>>>
>>> Le jeudi 27 juin 2019 à 20:55 +0200, Stefan Wahren a écrit :
>>>> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
>>>>
>>>> H264 header come from VC with 0 timestamps, which means they get a
>>>> strange timestamp when processed with VC/kernel start times,
>>>> particularly if used with the inline header option.
>>>> Remember the last frame timestamp and use that if set, or otherwise
>>>> use the kernel start time.
>>>
>>> Normally H264 headers are considered to be part of the following frame.
>>> Giving it the timestamp of the previous frame will likely confuse some
>>> userspace and cause an off-by-one in timestamp. I understand this is a
>>> workaround, but am wondering if this can be improved.
>>
>> Sorry, slight ambiguity in how I'm reading your comment.
>>
>> Are you saying that the header bytes want to be in the same buffer as
>> the following frame?
>> I thought this had also been discussed in the V4L2 stateful codec API
>> threads along with how many encoded frames were allowed in a single
>> V4L2 buffer. I certainly hadn't seen a statement about the header
>> bytes being combined with the next frame.
>> If the behaviour required by V4L2 is that header bytes and following
>> frame are in the same buffer, then that is relatively easy to achieve
>> in the firmware. This workaround can remain for older firmware as it
>> will never trigger if the firmware has combined the frames.
> 
> The frame alignment is a requirement specific to the stateful codec
> API.

Is it? I don't remember it being specified anywhere explicitly.
Here is the latest text:

https://hverkuil.home.xs4all.nl/codec-api/uapi/v4l/dev-encoder.html

I'll start a new thread about this, since this really needs to be
clarified.

Regards,

	Hans

 Stateful codec must interpret _H264 format as being one full frame
> per buffer (1 AU in progressive, and 1 to 2 AU in interlaced), the
> first frame should include SPS/PPS and any other prefix NALs. You may
> follow this rule in your capture driver if you want to make it possible
> to zero-copy the encoded frames from the capture to the decoder.
> Though, userspace will still have to parse as there is no indication
> for capture devices of the H264 alignment being used (that imply 1
> frame latency). Boris is working on a control for stateless CODEC to
> control if we are running in full-frame or per slices. I do hope this
> control will be extended later to allow cameras and decoders to signal
> their alignment, or simply to allow enabling low-latency modes
> supported by CODA and ZynMP firmwares.
> 
>>
>> Or are you saying that the header bytes remain in their own buffer,
>> but the timestamp wants to be the same as the next frame? That is
>> harder to achieve in the firmware, but could probably be done in the
>> kernel driver by holding on to the header bytes frame until the next
>> buffer had been received, at which point the timestamp can be copied
>> across. Possible, but just needs slightly careful handling to ensure
>> we don't lose buffers accidentally.
> 
> So this isn't specified by V4L2 itself. So instead I rely on H264 and
> MPEG TS specification to advance this. This is also the interpretation
> we have of timestamp in GStreamer (ffmpeg uses out-of-band headers with
> full frame AVC, so this does not apply).
> 
> So H264 AUD, SPS, PPS, SEI and other prefix NAL are considered to be
> the start of a frame. With this interpretation in mind, accumulating
> them is considered zero-latency. This basically means that if they are
> to have a timestamp, they would share that timestamp with all the
> slices of the same frame. In GStreamer, we have the notion of no
> timestamp, so in such a case we'd leave the timestamp empty and our
> parsers would pick the first valid timestamp that formed the full frame
> as being the frame timestamp (it's a bit buggier then that, but that's
> what it's suppose to do).
> 
> On top of that, if you don't have any meaningful alignment in your H264
> stream, the MPEG TS format states that the timestamp of a buffer should
> be the timestamp of the first NAL starting within this buffer, or the
> timestamp of the current NAL if there is not NAL start.
> 
> By respecting these standards you ensure that latency aware application
> can work with your driver without causing delays, or worst, having to
> deal with artificially late frames.
> 
> I hope this clarify and helps understand my request for "unhacking" the
> headers timestamps. I had assumed the timestamp came from the driver
> (instead of from the firmware), sorry if that caused confusion. If
> merging full frames is easier, I think I would opt for that as it's
> beneficial to performance when combined with other full frame APIs.
> 
> Nicolas
> 
>>
>>   Dave
>>
>>>> Link: https://github.com/raspberrypi/linux/issues/1836
>>>> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  .../staging/vc04_services/bcm2835-camera/bcm2835-camera.c  | 14 ++++++++++++--
>>>>  .../staging/vc04_services/bcm2835-camera/bcm2835-camera.h  |  2 ++
>>>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
>>>> index dce6e6d..0c04815 100644
>>>> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
>>>> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
>>>> @@ -359,7 +359,9 @@ static void buffer_cb(struct vchiq_mmal_instance *instance,
>>>>               }
>>>>       } else {
>>>>               if (dev->capture.frame_count) {
>>>> -                     if (dev->capture.vc_start_timestamp != -1 && pts) {
>>>> +                     if (dev->capture.vc_start_timestamp != -1) {
>>>> +                             buf->vb.vb2_buf.timestamp = ktime_get_ns();
>>>> +                     } else if (pts) {
>>>>                               ktime_t timestamp;
>>>>                               s64 runtime_us = pts -
>>>>                                   dev->capture.vc_start_timestamp;
>>>> @@ -372,8 +374,15 @@ static void buffer_cb(struct vchiq_mmal_instance *instance,
>>>>                                        ktime_to_ns(timestamp));
>>>>                               buf->vb.vb2_buf.timestamp = ktime_to_ns(timestamp);
>>>>                       } else {
>>>> -                             buf->vb.vb2_buf.timestamp = ktime_get_ns();
>>>> +                             if (dev->capture.last_timestamp) {
>>>> +                                     buf->vb.vb2_buf.timestamp =
>>>> +                                             dev->capture.last_timestamp;
>>>> +                             } else {
>>>> +                                     buf->vb.vb2_buf.timestamp =
>>>> +                                             ktime_to_ns(dev->capture.kernel_start_ts);
>>>> +                             }
>>>>                       }
>>>> +                     dev->capture.last_timestamp = buf->vb.vb2_buf.timestamp;
>>>>
>>>>                       vb2_set_plane_payload(&buf->vb.vb2_buf, 0, length);
>>>>                       vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>>>> @@ -541,6 +550,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>>>                        dev->capture.vc_start_timestamp, parameter_size);
>>>>
>>>>       dev->capture.kernel_start_ts = ktime_get();
>>>> +     dev->capture.last_timestamp = 0;
>>>>
>>>>       /* enable the camera port */
>>>>       dev->capture.port->cb_ctx = dev;
>>>> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
>>>> index 2b5679e..09273b0 100644
>>>> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
>>>> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
>>>> @@ -90,6 +90,8 @@ struct bm2835_mmal_dev {
>>>>               s64         vc_start_timestamp;
>>>>               /* Kernel start timestamp for streaming */
>>>>               ktime_t kernel_start_ts;
>>>> +             /* Timestamp of last frame */
>>>> +             u64             last_timestamp;
>>>>
>>>>               struct vchiq_mmal_port  *port; /* port being used for capture */
>>>>               /* camera port being used for capture */
>>>> --
>>>> 2.7.4
>>>>

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux