[FFmpeg-devel,2/3] mpegvideo_parser: parse the output picture number

Submitted by Jan Ekström on Feb. 11, 2018, 2:37 p.m.

Details

Message ID 20180211143752.7851-3-jeebjp@gmail.com
State New
Headers show

Commit Message

Jan Ekström Feb. 11, 2018, 2:37 p.m.
From: Masaki Tanaka <maki.rxrz@gmail.com>

Utilizes the temporal_reference field from the picture header.
---
 libavcodec/mpegvideo_parser.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Michael Niedermayer Feb. 11, 2018, 5:52 p.m.
On Sun, Feb 11, 2018 at 04:37:51PM +0200, Jan Ekström wrote:
> From: Masaki Tanaka <maki.rxrz@gmail.com>
> 
> Utilizes the temporal_reference field from the picture header.
> ---
>  libavcodec/mpegvideo_parser.c | 1 +
>  1 file changed, 1 insertion(+)

should be ok unless its intended to also restore the MSB

Theres no code using output_picture_number currently

thx

[...]
Jan Ekström Feb. 11, 2018, 6:31 p.m.
On Sun, Feb 11, 2018 at 7:52 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> should be ok unless its intended to also restore the MSB
>

The value seemed to be 10bit and if the pointer is at the position
right after the picture_start_code, then `buf[0] << 2` would move the
8 bits left of data two bits leftwards, and `buf[1] >> 6` would then
move the topmost bits of the following byte to be the bottom-most
bits.

Unless you mean that there's a forgotten `(uint16_t)` there as the end
value has 10 bits of effective data, making the correct way of doing
it something a la `(((uint16_t)buf[0]) << 2) | (((uint16_t)buf[1]) >>
6)`?

> Theres no code using output_picture_number currently
>

Seems like some API users utilize this value.


Jan
Jan Ekström March 5, 2018, 11:15 p.m.
On Sun, Feb 11, 2018 at 8:31 PM, Jan Ekström <jeebjp@gmail.com> wrote:
> On Sun, Feb 11, 2018 at 7:52 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
>>
>> should be ok unless its intended to also restore the MSB
>>
>
> The value seemed to be 10bit and if the pointer is at the position
> right after the picture_start_code, then `buf[0] << 2` would move the
> 8 bits left of data two bits leftwards, and `buf[1] >> 6` would then
> move the topmost bits of the following byte to be the bottom-most
> bits.
>
> Unless you mean that there's a forgotten `(uint16_t)` there as the end
> value has 10 bits of effective data, making the correct way of doing
> it something a la `(((uint16_t)buf[0]) << 2) | (((uint16_t)buf[1]) >>
> 6)`?
>

Ping regarding this part.

Jan
Michael Niedermayer March 7, 2018, 12:55 a.m.
On Sun, Feb 11, 2018 at 08:31:40PM +0200, Jan Ekström wrote:
> On Sun, Feb 11, 2018 at 7:52 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > should be ok unless its intended to also restore the MSB
> >
> 
> The value seemed to be 10bit and if the pointer is at the position
> right after the picture_start_code, then `buf[0] << 2` would move the
> 8 bits left of data two bits leftwards, and `buf[1] >> 6` would then
> move the topmost bits of the following byte to be the bottom-most
> bits.
> 
> Unless you mean that there's a forgotten `(uint16_t)` there as the end
> value has 10 bits of effective data, making the correct way of doing
> it something a la `(((uint16_t)buf[0]) << 2) | (((uint16_t)buf[1]) >>
> 6)`?

it was more a question about our API 

if output_picture_number is intended to be what it is called in english then
this patch is wrong as the number will frequently go back to 0 and
as more than 1024 pictures are output.
so taken as its english meaning, there would be code needed to increase(/decrease)
the MSB occasionally
OTOH if the field is intended to store a unspecified number of LSB only
i wonder what it could be used for

> 
> > Theres no code using output_picture_number currently
> >
> 
> Seems like some API users utilize this value.

What do they do with it ?

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
index 3406346a8b..4f554b684e 100644
--- a/libavcodec/mpegvideo_parser.c
+++ b/libavcodec/mpegvideo_parser.c
@@ -60,6 +60,7 @@  static void mpegvideo_extract_headers(AVCodecParserContext *s,
         switch(start_code) {
         case PICTURE_START_CODE:
             if (bytes_left >= 2) {
+                s->output_picture_number = (buf[0] << 2) | (buf[1] >> 6);
                 s->pict_type = (buf[1] >> 3) & 7;
                 if (bytes_left >= 4)
                 vbv_delay = ((buf[1] & 0x07) << 13) | (buf[2] << 5) | (buf[3]  >> 3);