Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dec265: support for per-image text dumping #201

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@chemag
Copy link

chemag commented Apr 4, 2019

No description provided.

@fancycode
Copy link
Member

fancycode left a comment

Thanks for your PR, I left some general comments / change requests. Also please rebase on master to get the fix for AppVeyor CI.

I'll let @farindk review the functionality.

Show resolved Hide resolved INSTALL Outdated
DE265_DECODER_PARAM_DISABLE_SAO=8 // (bool) disable SAO filter
//DE265_DECODER_PARAM_DISABLE_MC_RESIDUAL_IDCT=9, // (bool) disable decoding of IDCT residuals in MC blocks
//DE265_DECODER_PARAM_DISABLE_INTRA_RESIDUAL_IDCT=10 // (bool) disable decoding of IDCT residuals in MC blocks
DE265_DECODER_PARAM_DUMP_IMAGE_DATA=5,

This comment has been minimized.

Copy link
@fancycode

fancycode Apr 4, 2019

Member

The values of existing params should not change to keep compatibility with existing applications. Just add the new property at the end and assign a previously unused value.

This comment has been minimized.

Copy link
@chemag

chemag Apr 4, 2019

Author

done

Show resolved Hide resolved .gitignore

@fancycode fancycode requested a review from farindk Apr 4, 2019

@farindk

This comment has been minimized.

Copy link
Contributor

farindk commented Apr 4, 2019

Hello chemag,
thanks for your contribution.

I had a look at your code. The purpose of this appears to be to log a histogram of QP values used in an image.
I assume that you are using this for some research task or to analyze some encoder behavior. However, I do not think this is of general interest enough to include it in the master branch. To me, a branch in your research repository seems a better place to maintain this.
Alternatively, we could build a framework in libde265 to plug in analyzer-modules, like yours. But that is much more work to do (and at the moment not a high priority).

BTW: I noticed that when you build the histogram, you are not weighting the QPs by CU size. Please check whether you really intend it like this, or whether this is a bug.

Dirk

@chemag chemag force-pushed the chemag:master branch 2 times, most recently from ca0b3d5 to 09efac6 Apr 4, 2019

@chemag chemag closed this Apr 4, 2019

@chemag chemag force-pushed the chemag:master branch from 09efac6 to c172dd8 Apr 4, 2019

@chemag

This comment has been minimized.

Copy link
Author

chemag commented Apr 4, 2019

Hi Dirk,

Thanks for the quick review.

Hello chemag,
thanks for your contribution.

I had a look at your code. The purpose of this appears to be to log a histogram of QP values used in an image.
I assume that you are using this for some research task or to analyze some encoder behavior. However, I do not think this is of general interest enough to include it in the master branch. To me, a branch in your research repository seems a better place to maintain this.

The idea was to have a per-image dumper (which mostly coincides with a per-slice data dumper in most streams). QP values is what I care right now, but you could also e.g. add MVs there. I've been trying to script around these two for a while, and libde265 looks like the best FOSS hevc parser around for this.

Would it make more sense to add this as a separate tool under tools/ ? That seems to contain a hodge-podge of binaries.

Alternatively, we could build a framework in libde265 to plug in analyzer-modules, like yours. But that is much more work to do (and at the moment not a high priority).

BTW: I noticed that when you build the histogram, you are not weighting the QPs by CU size. Please check whether you really intend it like this, or whether this is a bug.
For my use case, it does not really matter. But for a more generic, I agree it's a bug. I'll fix it :)

Dirk

@chemag

This comment has been minimized.

Copy link
Author

chemag commented Apr 4, 2019

Hmm... Trying to reopen this

@chemag chemag reopened this Apr 4, 2019

@chemag chemag force-pushed the chemag:master branch from fc9df3f to 8f61dc0 Apr 4, 2019

@chemag

This comment has been minimized.

Copy link
Author

chemag commented Apr 15, 2019

Would it make more sense to add this as a separate tool under tools/ ? That seems to contain a hodge-podge of binaries.

Ping

@chemag chemag force-pushed the chemag:master branch from 8f61dc0 to 09efac6 Apr 19, 2019

chemag and others added some commits Apr 4, 2019

Chema Gonzalez
libde265: add callback mechanism to parse events
Add a mechanism that allows a decoder tool to get a callback for
every NAL unit, slice content, and image that it is interested on.
The idea is for the decoder tool to subscribe to the event it wants
to hear about, and then get the parser object.

This requires:

* replace C linkage with C++ linkage, so that we can add classes in the
callback
* move error codes to separate file, to break the circular dependency
between several files (e.g. `[sps|pps|vps].h`) and de265.h

Tested:

Wrote a decoder that subscribes to all the events, and just calls the
`dump` method for every object.
Chema Gonzalez
tools: add qpextract tool
Note that we have also defined the other callback functions, but disabled
them.

Tested:

```
$ ./tools/qpextract -dI -t 0 tears_400_x265.h265
qp_distro[29:34] { 174 552 397 241 137 62 }
qp_distro[32:40] { 85 325 297 191 63 15 0 0 5 }
qp_distro[38:41] { 227 174 138 28 }
qp_distro[39:42] { 89 147 159 91 }
qp_distro[39:42] { 88 171 144 56 }
...
```

@chemag chemag force-pushed the chemag:master branch from 177c154 to 6ae6891 Apr 19, 2019

@chemag chemag closed this Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.